[PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Grzegorz Nitka]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 35368 bytes --]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Hardware variants of E830 may support an unmanaged DPLL where the
configuration is hardcoded within the hardware and firmware, meaning
users cannot modify settings. However, users are able to check the DPLL
lock status and obtain configuration information through the Linux DPLL
and devlink health subsystem.

Availability of 'loss of lock' health status code determines if such
support is available, if true, register single DPLL device with 1 input
and 1 output and provide hardcoded/read only properties of a pin and
DPLL device. User is only allowed to check DPLL device status and receive
notifications on DPLL lock status change.

When present, the DPLL device locks to an external signal provided
through the PCIe/OCP pin. The expected input signal is 1PPS
(1 Pulse Per Second) embedded on a 10MHz reference clock.
The DPLL produces output:
- for MAC (Media Access Control) & PHY (Physical Layer) clocks,
- 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
---
v5:
- rebased (baseline does not include dependent e825C patches now)
- added health status notification (thru devlink and DPLL subsystem)
v4:
- add correct strcuture for reading supported health status codes and
  use it to parse the outcome of 0xFF21 AQ command.
---
 .../device_drivers/ethernet/intel/ice.rst     |  83 +++++
 .../net/ethernet/intel/ice/devlink/health.c   |   4 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  12 +
 drivers/net/ethernet/intel/ice/ice_common.c   | 135 ++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |   8 +
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 311 ++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_dpll.h     |  11 +
 drivers/net/ethernet/intel/ice/ice_main.c     |  14 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  46 +++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |   1 +
 10 files changed, 602 insertions(+), 23 deletions(-)

diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
index 0bca293cf9cb..c5cf9af75ca9 100644
--- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
+++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
@@ -941,6 +941,89 @@ To see input signal on those PTP pins, you need to configure DPLL properly.
 Output signal is only visible on DPLL and to send it to the board SMA/U.FL pins,
 DPLL output pins have to be manually configured.
 
+Unmanaged DPLL Support
+----------------------
+Hardware variants of E830 may support an unmanaged DPLL:
+
+- Intel® Ethernet Network Adapter E830-XXVDA8F for OCP 3.0,
+
+- Intel® Ethernet Network Adapter E830-XXVDA4F.
+
+In the case of the unmanaged DPLL, the configuration is hardcoded within the
+hardware and firmware, meaning users cannot modify settings. However,
+users can check the DPLL lock status and obtain configuration information
+through the Linux DPLL subsystem.
+
+When present, the DPLL device locks to an external signal provided through the
+PCIe/OCP pin. The expected input signal is 1PPS (1 Pulse Per Second) embedded
+on a 10MHz reference clock.
+The DPLL produces output:
+
+- for MAC (Media Access Control) & PHY (Physical Layer) clocks,
+
+- 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.
+
+Requirements: The Linux kernel must have support for both the DPLL Subsystem
+and the Embedded Sync patch series.
+
+Example output of querying the Linux DPLL subsystem can be found below.
+
+.. code-block:: console
+  :caption: Dumping the DPLL pins
+
+  $ <ynl> --spec Documentation/netlink/specs/dpll.yaml --dump pin-get
+  [{'board-label': '1588-TIME_SYNC',
+    'capabilities': set(),
+    'clock-id': 282574471561216,
+    'esync-frequency': 1,
+    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min': 1}],
+    'esync-pulse': 25,
+    'frequency': 10000000,
+    'id': 13,
+    'module-name': 'ice',
+    'parent-device': [{'direction': 'input',
+                       'parent-id': 6,
+                       'state': 'connected'}],
+    'phase-adjust-max': 0,
+    'phase-adjust-min': 0,
+    'type': 'ext'},
+    {'board-label': 'MAC-PHY-CLK',
+      'capabilities': set(),
+    'clock-id': 282574471561216,
+    'frequency': 156250000,
+    'id': 14,
+    'module-name': 'ice',
+    'parent-device': [{'direction': 'output',
+                       'parent-id': 6,
+                       'state': 'connected'}],
+    'phase-adjust-max': 0,
+    'phase-adjust-min': 0,
+    'type': 'synce-eth-port'},
+  {'board-label': '1588-TIME_REF',
+    'capabilities': set(),
+    'clock-id': 282574471561216,
+    'frequency': 1,
+    'id': 15,
+    'module-name': 'ice',
+    'parent-device': [{'direction': 'output',
+                       'parent-id': 6,
+                       'state': 'connected'}],
+    'phase-adjust-max': 0,
+    'phase-adjust-min': 0,
+    'type': 'int-oscillator'}]
+
+.. code-block:: console
+  :caption: Dumping the DPLL devices
+
+  $ <ynl> --spec Documentation/netlink/specs/dpll.yaml --dump device-get
+  [{'clock-id': 282574471561216,
+    'id': 6,
+    'lock-status': 'locked',
+    'mode': 'automatic',
+    'mode-supported': ['automatic'],
+    'module-name': 'ice',
+    'type': 'eec'}]
+
 GNSS module
 -----------
 Requires kernel compiled with CONFIG_GNSS=y or CONFIG_GNSS=m.
diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
index 8e9a8a8178d4..31e6c5107c97 100644
--- a/drivers/net/ethernet/intel/ice/devlink/health.c
+++ b/drivers/net/ethernet/intel/ice/devlink/health.c
@@ -101,6 +101,8 @@ static const struct ice_health_status ice_health_status_lookup[] = {
 		"Supplied MIB file is invalid. DCB reverted to default configuration.",
 		"Disable FW-LLDP and check DCBx system configuration.",
 		{ice_port_number_label, "MIB ID"}},
+	{ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK, "Local DPLL lock status",
+		NULL,},
 };
 
 static int ice_health_status_lookup_compare(const void *a, const void *b)
@@ -242,6 +244,8 @@ void ice_process_health_status_event(struct ice_pf *pf, struct ice_rq_event_info
 				pf->health_reporters.fw_status = *health_info;
 				devlink_health_report(pf->health_reporters.fw,
 						      "FW syndrome reported", NULL);
+				if (status_code == ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK)
+					ice_dpll_lock_state_set_unmanaged(pf, health_info, true);
 				break;
 			case ICE_AQC_HEALTH_STATUS_PF:
 			case ICE_AQC_HEALTH_STATUS_PORT:
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 859e9c66f3e7..3f8c1b8befc3 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1498,6 +1498,7 @@ struct ice_aqc_get_link_topo {
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575		0x21
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL30632_80032	0x24
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_SI5383_5384	0x25
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640		0x27
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_E822_PHY		0x30
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827		0x31
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX	0x47
@@ -2481,11 +2482,14 @@ enum ice_aqc_health_status {
 	ICE_AQC_HEALTH_STATUS_ERR_BMC_RESET			= 0x50B,
 	ICE_AQC_HEALTH_STATUS_ERR_LAST_MNG_FAIL			= 0x50C,
 	ICE_AQC_HEALTH_STATUS_ERR_RESOURCE_ALLOC_FAIL		= 0x50D,
+	ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK			= 0x601,
 	ICE_AQC_HEALTH_STATUS_ERR_FW_LOOP			= 0x1000,
 	ICE_AQC_HEALTH_STATUS_ERR_FW_PFR_FAIL			= 0x1001,
 	ICE_AQC_HEALTH_STATUS_ERR_LAST_FAIL_AQ			= 0x1002,
 };
 
+#define ICE_AQC_HEALTH_STATUS_CODE_NUM				64
+
 /* Get Health Status (indirect 0xFF22) */
 struct ice_aqc_get_health_status {
 	__le16 health_status_count;
@@ -2512,6 +2516,13 @@ struct ice_aqc_health_status_elem {
 	__le32 internal_data2;
 };
 
+/* Get Health Status response buffer entry, (0xFF21)
+ * repeated per reported health status
+ */
+struct ice_aqc_health_status_supp_elem {
+	__le16 health_status_code;
+};
+
 /* Admin Queue command opcodes */
 enum ice_adminq_opc {
 	/* AQ commands */
@@ -2675,6 +2686,7 @@ enum ice_adminq_opc {
 
 	/* System Diagnostic commands */
 	ice_aqc_opc_set_health_status_cfg		= 0xFF20,
+	ice_aqc_opc_get_supported_health_status_codes	= 0xFF21,
 	ice_aqc_opc_get_health_status			= 0xFF22,
 
 	/* FW Logging Commands */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index c0a19f232538..5e1a18380936 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -3044,6 +3044,29 @@ bool ice_is_cgu_in_netlist(struct ice_hw *hw)
 	return false;
 }
 
+/**
+ * ice_is_unmanaged_cgu_in_netlist - check for unmanaged CGU presence
+ * @hw: pointer to the hw struct
+ *
+ * Check if the unmanaged Clock Generation Unit (CGU) device is present in the netlist.
+ * Save the CGU part number in the hw structure for later use.
+ * Return:
+ * * true - unmanaged cgu is present
+ * * false - unmanaged cgu is not present
+ */
+bool ice_is_unmanaged_cgu_in_netlist(struct ice_hw *hw)
+{
+	if (!ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+				   ICE_AQC_LINK_TOPO_NODE_CTX_GLOBAL,
+				   ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640,
+				   NULL)) {
+		hw->cgu_part_number = ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640;
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * ice_is_gps_in_netlist
  * @hw: pointer to the hw struct
@@ -6306,6 +6329,118 @@ bool ice_is_fw_health_report_supported(struct ice_hw *hw)
 				     ICE_FW_API_HEALTH_REPORT_PATCH);
 }
 
+/**
+ * ice_aq_get_health_status_supported - get supported health status codes
+ * @hw: pointer to the HW struct
+ * @buff: pointer to buffer where health status elements will be stored
+ * @num: number of health status elements buffer can hold
+ *
+ * Return:
+ * * 0 - success,
+ * * negative - AQ error code.
+ */
+static int
+ice_aq_get_health_status_supported(struct ice_hw *hw,
+				   struct ice_aqc_health_status_supp_elem *buff,
+				   int num)
+{
+	u16 code = ice_aqc_opc_get_supported_health_status_codes;
+	struct libie_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, code);
+
+	return ice_aq_send_cmd(hw, &desc, buff, num * sizeof(*buff), NULL);
+}
+
+/**
+ * ice_aq_get_health_status - get current health status array from the firmware
+ * @hw: pointer to the HW struct
+ * @buff: pointer to buffer where health status elements will be stored
+ * @num: number of health status elements buffer can hold
+ *
+ * Return:
+ * * 0 - success,
+ * * negative - AQ error code.
+ */
+int ice_aq_get_health_status(struct ice_hw *hw,
+			     struct ice_aqc_health_status_elem *buff, int num)
+{
+	struct libie_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc,
+				      ice_aqc_opc_get_health_status);
+
+	return ice_aq_send_cmd(hw, &desc, buff, num * sizeof(*buff), NULL);
+}
+
+/**
+ * ice_is_health_status_code_supported - check if health status code is supported
+ * @hw: pointer to the hardware structure
+ * @code: health status code to check
+ * @supported: pointer to boolean result
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
+					bool *supported)
+{
+	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+	struct ice_aqc_health_status_supp_elem *buff;
+	int ret;
+
+	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+	ret = ice_aq_get_health_status_supported(hw, buff, BUFF_SIZE);
+	if (ret)
+		goto free_buff;
+	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+		if (le16_to_cpu(buff[i].health_status_code) == code) {
+			*supported = true;
+			break;
+		}
+
+free_buff:
+	kfree(buff);
+	return ret;
+}
+
+/**
+ * ice_get_last_health_status_code - get last health status for given code
+ * @hw: pointer to the hardware structure
+ * @out: pointer to the health status struct to be filled
+ * @code: health status code to check
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+int ice_get_last_health_status_code(struct ice_hw *hw,
+				    struct ice_aqc_health_status_elem *out,
+				    u16 code)
+{
+	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+	struct ice_aqc_health_status_elem *buff;
+	int ret, last_status = -1;
+
+	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+	ret = ice_aq_get_health_status(hw, buff, BUFF_SIZE);
+	if (ret)
+		goto free_buff;
+	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+		if (le16_to_cpu(buff[i].health_status_code) == code)
+			last_status = i;
+
+	if (last_status >= 0)
+		memcpy(out, &buff[last_status], sizeof(*out));
+	else
+		memset(out, 0, sizeof(*out));
+
+free_buff:
+	kfree(buff);
+	return ret;
+}
+
 /**
  * ice_aq_set_health_status_cfg - Configure FW health events
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index e700ac0dc347..cbecee66e2a7 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -162,6 +162,7 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
 bool ice_is_phy_rclk_in_netlist(struct ice_hw *hw);
 bool ice_is_clock_mux_in_netlist(struct ice_hw *hw);
 bool ice_is_cgu_in_netlist(struct ice_hw *hw);
+bool ice_is_unmanaged_cgu_in_netlist(struct ice_hw *hw);
 bool ice_is_gps_in_netlist(struct ice_hw *hw);
 int
 ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
@@ -188,6 +189,13 @@ ice_get_link_default_override(struct ice_link_default_override_tlv *ldo,
 			      struct ice_port_info *pi);
 bool ice_is_phy_caps_an_enabled(struct ice_aqc_get_phy_caps_data *caps);
 bool ice_is_fw_health_report_supported(struct ice_hw *hw);
+int ice_aq_get_health_status(struct ice_hw *hw,
+			     struct ice_aqc_health_status_elem *buff, int num);
+int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
+					bool *supported);
+int ice_get_last_health_status_code(struct ice_hw *hw,
+				    struct ice_aqc_health_status_elem *out,
+				    u16 code);
 int ice_aq_set_health_status_cfg(struct ice_hw *hw, u8 event_source);
 int ice_aq_get_phy_equalization(struct ice_hw *hw, u16 data_in, u16 op_code,
 				u8 serdes_num, int *output);
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 53b54e395a2e..c53ce0e4e804 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -17,6 +17,8 @@
 #define ICE_DPLL_SW_PIN_INPUT_BASE_SFP		4
 #define ICE_DPLL_SW_PIN_INPUT_BASE_QSFP		6
 #define ICE_DPLL_SW_PIN_OUTPUT_BASE		0
+#define ICE_DPLL_HEALTH_STATUS_LOCKED		1
+#define ICE_DPLL_HEALTH_STATUS_UNLOCKED		0
 
 #define ICE_DPLL_PIN_SW_INPUT_ABS(in_idx) \
 	(ICE_DPLL_SW_PIN_INPUT_BASE_SFP + (in_idx))
@@ -79,6 +81,10 @@ static const struct dpll_pin_frequency ice_esync_range[] = {
 	DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ),
 };
 
+static const struct dpll_pin_frequency ice_esync_range_unmanaged[] = {
+	DPLL_PIN_FREQUENCY_1PPS,
+};
+
 /**
  * ice_dpll_is_sw_pin - check if given pin shall be controlled by SW
  * @pf: private board structure
@@ -1008,9 +1014,11 @@ ice_dpll_pin_state_get(const struct dpll_pin *pin, void *pin_priv,
 		return -EBUSY;
 
 	mutex_lock(&pf->dplls.lock);
-	ret = ice_dpll_pin_state_update(pf, p, pin_type, extack);
-	if (ret)
-		goto unlock;
+	if (!pf->dplls.unmanaged) {
+		ret = ice_dpll_pin_state_update(pf, p, pin_type, extack);
+		if (ret)
+			goto unlock;
+	}
 	if (pin_type == ICE_DPLL_PIN_TYPE_INPUT ||
 	    pin_type == ICE_DPLL_PIN_TYPE_OUTPUT)
 		*state = p->state[d->dpll_idx];
@@ -2036,9 +2044,12 @@ ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv,
 		mutex_unlock(&pf->dplls.lock);
 		return -EOPNOTSUPP;
 	}
-	esync->range = ice_esync_range;
+	if (pf->dplls.unmanaged)
+		esync->range = ice_esync_range_unmanaged;
+	else
+		esync->range = ice_esync_range;
 	esync->range_num = ARRAY_SIZE(ice_esync_range);
-	if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
+	if (p->flags[0] & ICE_DPLL_IN_ESYNC_ENABLED) {
 		esync->freq = DPLL_PIN_FREQUENCY_1_HZ;
 		esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT;
 	} else {
@@ -2436,6 +2447,21 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
 	.esync_get = ice_dpll_output_esync_get,
 };
 
+static const struct dpll_pin_ops ice_dpll_input_unmanaged_ops = {
+	.frequency_get = ice_dpll_input_frequency_get,
+	.direction_get = ice_dpll_input_direction,
+	.state_on_dpll_get = ice_dpll_input_state_get,
+#if defined(HAVE_DPLL_ESYNC)
+	.esync_get = ice_dpll_input_esync_get,
+#endif /* HAVE_DPLL_ESYNC */
+};
+
+static const struct dpll_pin_ops ice_dpll_output_unmanaged_ops = {
+	.frequency_get = ice_dpll_output_frequency_get,
+	.direction_get = ice_dpll_output_direction,
+	.state_on_dpll_get = ice_dpll_output_state_get,
+};
+
 static const struct dpll_device_ops ice_dpll_ops = {
 	.lock_status_get = ice_dpll_lock_status_get,
 	.mode_get = ice_dpll_mode_get,
@@ -3000,9 +3026,11 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
 		ret = ice_dpll_register_pins(first, pins, ops, count);
 		if (ret)
 			goto release_pins;
-		ret = ice_dpll_register_pins(second, pins, ops, count);
-		if (ret)
-			goto unregister_first;
+		if (second) {
+			ret = ice_dpll_register_pins(second, pins, ops, count);
+			if (ret)
+				goto unregister_first;
+		}
 	}
 
 	return 0;
@@ -3112,6 +3140,18 @@ static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
 	struct ice_dpll *de = &d->eec;
 	struct ice_dpll *dp = &d->pps;
 
+	if (d->unmanaged) {
+		ice_dpll_unregister_pins(de->dpll, inputs,
+					 &ice_dpll_input_unmanaged_ops,
+					 num_inputs);
+		ice_dpll_unregister_pins(de->dpll, outputs,
+					 &ice_dpll_output_unmanaged_ops,
+					 num_outputs);
+		ice_dpll_release_pins(inputs, num_inputs);
+		ice_dpll_release_pins(outputs, num_outputs);
+		return;
+	}
+
 	ice_dpll_deinit_rclk_pin(pf);
 	if (cgu) {
 		ice_dpll_unregister_pins(dp->dpll, inputs, &ice_dpll_input_ops,
@@ -3155,24 +3195,35 @@ 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;
 
+	if (!pf->dplls.unmanaged) {
+		input_ops = &ice_dpll_input_ops;
+		output_ops = &ice_dpll_output_ops;
+	} else {
+		input_ops = &ice_dpll_input_unmanaged_ops;
+		output_ops = &ice_dpll_output_unmanaged_ops;
+	}
+
 	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;
-	if (cgu) {
+	if (cgu || pf->dplls.unmanaged) {
 		ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.outputs,
 						count,
 						pf->dplls.num_outputs,
-						&ice_dpll_output_ops,
+						output_ops,
 						pf->dplls.eec.dpll,
 						pf->dplls.pps.dpll);
 		if (ret)
 			goto deinit_inputs;
+		if (pf->dplls.unmanaged)
+			return 0;
 		count += pf->dplls.num_outputs;
 		if (!pf->dplls.generic) {
 			ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.sma,
@@ -3224,11 +3275,11 @@ static int ice_dpll_init_pins(struct ice_pf *pf, bool cgu)
 deinit_outputs:
 	ice_dpll_deinit_direct_pins(cgu, pf->dplls.outputs,
 				    pf->dplls.num_outputs,
-				    &ice_dpll_output_ops, pf->dplls.pps.dpll,
+				    output_ops, pf->dplls.pps.dpll,
 				    pf->dplls.eec.dpll);
 deinit_inputs:
 	ice_dpll_deinit_direct_pins(cgu, pf->dplls.inputs, pf->dplls.num_inputs,
-				    &ice_dpll_input_ops, pf->dplls.pps.dpll,
+				    input_ops, pf->dplls.pps.dpll,
 				    pf->dplls.eec.dpll);
 	return ret;
 }
@@ -3284,7 +3335,8 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
 
 		if (type == DPLL_TYPE_PPS && ice_dpll_is_pps_phase_monitor(pf))
 			ops =  &ice_dpll_pom_ops;
-		ice_dpll_update_state(pf, d, true);
+		if (!pf->dplls.unmanaged)
+			ice_dpll_update_state(pf, d, true);
 		ret = dpll_device_register(d->dpll, type, ops, d);
 		if (ret) {
 			dpll_device_put(d->dpll);
@@ -3310,6 +3362,33 @@ static void ice_dpll_deinit_worker(struct ice_pf *pf)
 	kthread_destroy_worker(d->kworker);
 }
 
+/**
+ * ice_dpll_pin_freq_info - find pin frequency from supported ones
+ * @hw: pointer to the hardware structure
+ * @pin_idx: pin index
+ * @input: if input pin
+ *
+ * This function searches through the array of supported frequencies for a
+ * DPLL pin and returns single frequency pin is capable, if pin support only
+ * one frequency. Shall be used only for dpll with driver hardcoded frequency.
+ *
+ * Return:
+ * * 0 - failure, pin uses multiple frequencies,
+ * * frequency - success.
+ */
+static u64 ice_dpll_pin_freq_info(struct ice_hw *hw, u8 pin_idx, bool input)
+{
+	struct dpll_pin_frequency *freqs;
+	u8 freq_num;
+
+	/* Get supported frequencies for this pin */
+	freqs = ice_cgu_get_pin_freq_supp(hw, pin_idx, input, &freq_num);
+	if (!freqs || freq_num != 1 || freqs[0].min != freqs[0].max)
+		return 0;
+
+	return freqs[0].min;
+}
+
 /**
  * ice_dpll_init_worker - Initialize DPLLs periodic worker
  * @pf: board private structure
@@ -3469,6 +3548,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 		pins[i].prop.board_label = ice_cgu_get_pin_name(hw, i, input);
 		pins[i].prop.type = ice_cgu_get_pin_type(hw, i, input);
 		if (input) {
+			if (pf->dplls.unmanaged) {
+				pins[i].freq = ice_dpll_pin_freq_info(hw, i,
+								      input);
+				pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
+				pins[i].status =
+					ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP;
+				pins[i].flags[0] = ICE_DPLL_IN_ESYNC_ENABLED;
+				continue;
+			}
 			ret = ice_aq_get_cgu_ref_prio(hw, de->dpll_idx, i,
 						      &de->input_prio[i]);
 			if (ret)
@@ -3482,6 +3570,12 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 			if (ice_dpll_is_sw_pin(pf, i, true))
 				pins[i].hidden = true;
 		} else {
+			if (pf->dplls.unmanaged) {
+				pins[i].freq = ice_dpll_pin_freq_info(hw, i,
+								      input);
+				pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
+				continue;
+			}
 			ret = ice_cgu_get_output_pin_state_caps(hw, i, &caps);
 			if (ret)
 				return ret;
@@ -3499,10 +3593,13 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 		pins[i].prop.freq_supported_num = freq_supp_num;
 		pins[i].pf = pf;
 	}
-	if (input)
+	if (input && !pf->dplls.unmanaged) {
 		ret = ice_dpll_init_ref_sync_inputs(pf);
+		if (ret)
+			return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 /**
@@ -3653,6 +3750,81 @@ static void ice_dpll_deinit_info(struct ice_pf *pf)
 	kfree(pf->dplls.pps.input_prio);
 }
 
+/**
+ * ice_dpll_lock_state_init_unmanaged - initialize lock state for unmanaged dpll
+ * @pf: board private structure
+ *
+ * Initialize the lock state for unmanaged DPLL by checking health status.
+ * For unmanaged DPLL, we rely on hardware autonomous operation.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - init failure reason
+ */
+static int ice_dpll_lock_state_init_unmanaged(struct ice_pf *pf)
+{
+	u16 code = ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK;
+	struct ice_aqc_health_status_elem buff;
+	int ret;
+
+	ret = ice_get_last_health_status_code(&pf->hw, &buff, code);
+	if (ret)
+		return ret;
+	ice_dpll_lock_state_set_unmanaged(pf, &buff, false);
+
+	return ret;
+}
+
+/**
+ * ice_dpll_init_info_unmanaged - init dpll information for unmanaged dpll
+ * @pf: board private structure
+ *
+ * Acquire (from HW) and set basic dpll information (on pf->dplls struct).
+ * For unmanaged dpll mode.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - init failure reason
+ */
+static int ice_dpll_init_info_unmanaged(struct ice_pf *pf)
+{
+	struct ice_dplls *d = &pf->dplls;
+	struct ice_dpll *de = &d->eec;
+	int ret = 0;
+
+	d->clock_id = ice_generate_clock_id(pf);
+	d->num_inputs = ice_cgu_get_pin_num(&pf->hw, true);
+	d->num_outputs = ice_cgu_get_pin_num(&pf->hw, false);
+	ice_dpll_lock_state_init_unmanaged(pf);
+	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;
+
+	d->outputs = kcalloc(d->num_outputs, sizeof(*d->outputs), GFP_KERNEL);
+	if (!d->outputs) {
+		ret = -ENOMEM;
+		goto deinit_info;
+	}
+
+	ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_OUTPUT);
+	if (ret)
+		goto deinit_info;
+
+	de->mode = DPLL_MODE_AUTOMATIC;
+	dev_dbg(ice_pf_to_dev(pf), "%s - success, inputs:%u, outputs:%u\n",
+		__func__, d->num_inputs, d->num_outputs);
+	return 0;
+deinit_info:
+	dev_err(ice_pf_to_dev(pf), "%s - fail: d->inputs:%p, d->outputs:%p\n",
+		__func__, d->inputs, d->outputs);
+	ice_dpll_deinit_info(pf);
+	return ret;
+}
+
 /**
  * ice_dpll_init_info - prepare pf's dpll information structure
  * @pf: board private structure
@@ -3686,6 +3858,7 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
 	dp->dpll_idx = abilities.pps_dpll_idx;
 	d->num_inputs = abilities.num_inputs;
 	d->num_outputs = abilities.num_outputs;
+
 	d->input_phase_adj_max = le32_to_cpu(abilities.max_in_phase_adj) &
 		ICE_AQC_GET_CGU_MAX_PHASE_ADJ;
 	d->output_phase_adj_max = le32_to_cpu(abilities.max_out_phase_adj) &
@@ -3752,6 +3925,42 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
 	return ret;
 }
 
+/**
+ * ice_dpll_lock_state_set_unmanaged - determine lock state from health status
+ * @pf: board private structure
+ * @buff: health status buffer
+ * @notify: if true, notify dpll device
+ *
+ * Set unmanaged dpll lock state based on health status code and internal data.
+ * Context: Acquires and releases pf->dplls.lock (must release before notify
+ * if called).
+ */
+void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+				       const struct ice_aqc_health_status_elem *buff,
+				       bool notify)
+{
+	u32 internal_data = le32_to_cpu(buff->internal_data1);
+	struct ice_dpll *d = &pf->dplls.eec;
+
+	if (!ice_pf_src_tmr_owned(pf))
+		return;
+
+	mutex_lock(&pf->dplls.lock);
+	if (buff->health_status_code == 0 ||
+	    internal_data == ICE_DPLL_HEALTH_STATUS_LOCKED)
+		d->dpll_state = DPLL_LOCK_STATUS_LOCKED;
+	else
+		d->dpll_state = DPLL_LOCK_STATUS_UNLOCKED;
+
+	if (d->prev_dpll_state == d->dpll_state)
+		notify = false;
+	else
+		d->prev_dpll_state = d->dpll_state;
+	mutex_unlock(&pf->dplls.lock);
+	if (notify && d->dpll)
+		dpll_device_change_ntf(d->dpll);
+}
+
 /**
  * ice_dpll_deinit - Disable the driver/HW support for dpll subsystem
  * the dpll device.
@@ -3771,15 +3980,55 @@ void ice_dpll_deinit(struct ice_pf *pf)
 	if (cgu)
 		ice_dpll_deinit_worker(pf);
 
-	ice_dpll_deinit_pins(pf, cgu);
-	ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
-	ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
+	ice_dpll_deinit_pins(pf, cgu || pf->dplls.unmanaged);
+	if (pf->dplls.pps.dpll)
+		ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
+	ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu || pf->dplls.unmanaged);
 	ice_dpll_deinit_info(pf);
 	mutex_destroy(&pf->dplls.lock);
 }
 
 /**
- * ice_dpll_init - initialize support for dpll subsystem
+ * ice_dpll_init_unmanaged - initialize support for unmanaged dpll subsystem
+ * @pf: board private structure
+ *
+ * Set up the device dplls for unmanaged mode, register them and pins connected
+ * within Linux dpll subsystem. Allow userspace to obtain state of DPLL.
+ *
+ * Context: Initializes pf->dplls.lock mutex.
+ */
+static void ice_dpll_init_unmanaged(struct ice_pf *pf)
+{
+	struct ice_dplls *d = &pf->dplls;
+	int err;
+
+	if (!ice_pf_src_tmr_owned(pf))
+		return;
+	err = ice_dpll_init_info_unmanaged(pf);
+	if (err)
+		goto err_exit;
+	mutex_init(&d->lock);
+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, true, DPLL_TYPE_EEC);
+	if (err)
+		goto deinit_info;
+	err = ice_dpll_init_pins(pf, true);
+	if (err)
+		goto deinit_eec;
+	set_bit(ICE_FLAG_DPLL, pf->flags);
+
+	return;
+
+deinit_eec:
+	ice_dpll_deinit_dpll(pf, &pf->dplls.eec, true);
+deinit_info:
+	ice_dpll_deinit_info(pf);
+	mutex_destroy(&d->lock);
+err_exit:
+	dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
+}
+
+/**
+ * ice_dpll_init_managed - initialize support for managed dpll subsystem
  * @pf: board private structure
  *
  * Set up the device dplls, register them and pins connected within Linux dpll
@@ -3788,7 +4037,7 @@ void ice_dpll_deinit(struct ice_pf *pf)
  *
  * Context: Initializes pf->dplls.lock mutex.
  */
-void ice_dpll_init(struct ice_pf *pf)
+static void ice_dpll_init_managed(struct ice_pf *pf)
 {
 	bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
 	struct ice_dplls *d = &pf->dplls;
@@ -3828,3 +4077,21 @@ void ice_dpll_init(struct ice_pf *pf)
 	mutex_destroy(&d->lock);
 	dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
 }
+
+/**
+ * ice_dpll_init - initialize support for dpll subsystem
+ * @pf: board private structure
+ *
+ * Set up the device dplls, register them and pins connected within Linux dpll
+ * subsystem. Allow userspace to obtain state of DPLL and handling of DPLL
+ * configuration requests.
+ *
+ * Context: Initializes pf->dplls.lock mutex.
+ */
+void ice_dpll_init(struct ice_pf *pf)
+{
+	if (!pf->dplls.unmanaged)
+		ice_dpll_init_managed(pf);
+	else
+		ice_dpll_init_unmanaged(pf);
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index c0da03384ce9..fa61b4178b3c 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -8,6 +8,9 @@
 
 #define ICE_DPLL_RCLK_NUM_MAX	4
 
+#define ICE_DPLL_UNMANAGED_PIN_NUM	4
+#define ICE_DPLL_IN_ESYNC_ENABLED	ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN
+
 /**
  * enum ice_dpll_pin_sw - enumerate ice software pin indices:
  * @ICE_DPLL_PIN_SW_1_IDX: index of first SW pin
@@ -132,14 +135,22 @@ struct ice_dplls {
 	s32 output_phase_adj_max;
 	u32 periodic_counter;
 	bool generic;
+	bool unmanaged;
 };
 
 #if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
 void ice_dpll_init(struct ice_pf *pf);
 void ice_dpll_deinit(struct ice_pf *pf);
+void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+				       const struct ice_aqc_health_status_elem *buff,
+				       bool notify);
 #else
 static inline void ice_dpll_init(struct ice_pf *pf) { }
 static inline void ice_dpll_deinit(struct ice_pf *pf) { }
+static inline void
+ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+				  const struct ice_aqc_health_status_elem *buff,
+				  bool notify) { }
 #endif
 
 #endif
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0b6175ade40d..b16ede1f139d 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4797,8 +4797,20 @@ static void ice_init_features(struct ice_pf *pf)
 	if (ice_is_feature_supported(pf, ICE_F_GNSS))
 		ice_gnss_init(pf);
 
+	/* Initialize unmanaged DPLL detection */
+	{
+		u16 code = ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK;
+		int err;
+
+		err = ice_is_health_status_code_supported(&pf->hw, code,
+							  &pf->dplls.unmanaged);
+		if (err || !ice_is_unmanaged_cgu_in_netlist(&pf->hw))
+			pf->dplls.unmanaged = false;
+	}
+
 	if (ice_is_feature_supported(pf, ICE_F_CGU) ||
-	    ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
+	    ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
+	    pf->dplls.unmanaged)
 		ice_dpll_init(pf);
 
 	/* Note: Flow director init failure is non-fatal to load */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 35680dbe4a7f..4abbb9fd607d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -20,6 +20,10 @@ static struct dpll_pin_frequency ice_cgu_pin_freq_10_mhz[] = {
 	DPLL_PIN_FREQUENCY_10MHZ,
 };
 
+static struct dpll_pin_frequency ice_cgu_pin_freq_156_25mhz[] = {
+	DPLL_PIN_FREQUENCY_RANGE(156250000, 156250000),
+};
+
 static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
 	{ "CVL-SDP22",	  ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
 		ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
@@ -131,6 +135,18 @@ static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
 	{ "NONE",	   ZL_OUT5, 0, 0 },
 };
 
+static const struct ice_cgu_pin_desc ice_e830_unmanaged_inputs[] = {
+	{ "1588-TIME_SYNC", 0, DPLL_PIN_TYPE_EXT,
+	  ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
+};
+
+static const struct ice_cgu_pin_desc ice_e830_unmanaged_outputs[] = {
+	{ "MAC-PHY-CLK", 0, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+	  ARRAY_SIZE(ice_cgu_pin_freq_156_25mhz), ice_cgu_pin_freq_156_25mhz },
+	{ "1588-TIME_REF", 1, DPLL_PIN_TYPE_INT_OSCILLATOR,
+	  ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz},
+};
+
 /* Low level functions for interacting with and managing the device clock used
  * for the Precision Time Protocol.
  *
@@ -5684,6 +5700,24 @@ ice_cgu_get_pin_desc(struct ice_hw *hw, bool input, int *size)
 	case ICE_DEV_ID_E823C_SGMII:
 		t = ice_cgu_get_pin_desc_e823(hw, input, size);
 		break;
+	case ICE_DEV_ID_E830CC_BACKPLANE:
+	case ICE_DEV_ID_E830CC_QSFP56:
+	case ICE_DEV_ID_E830CC_SFP:
+	case ICE_DEV_ID_E830CC_SFP_DD:
+	case ICE_DEV_ID_E830C_BACKPLANE:
+	case ICE_DEV_ID_E830C_QSFP:
+	case ICE_DEV_ID_E830C_SFP:
+	case ICE_DEV_ID_E830_XXV_BACKPLANE:
+	case ICE_DEV_ID_E830_XXV_QSFP:
+	case ICE_DEV_ID_E830_XXV_SFP:
+		if (input) {
+			t = ice_e830_unmanaged_inputs;
+			*size = ARRAY_SIZE(ice_e830_unmanaged_inputs);
+		} else {
+			t = ice_e830_unmanaged_outputs;
+			*size = ARRAY_SIZE(ice_e830_unmanaged_outputs);
+		}
+		break;
 	default:
 		break;
 	}
@@ -5710,6 +5744,18 @@ int ice_cgu_get_num_pins(struct ice_hw *hw, bool input)
 	return 0;
 }
 
+/**
+ * ice_cgu_get_pin_num - get pin description array size
+ * @hw: pointer to the hw struct
+ * @input: if request is done against input or output pins
+ *
+ * Return: size of pin description array for given hw.
+ */
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input)
+{
+	return ice_cgu_get_num_pins(hw, input);
+}
+
 /**
  * ice_cgu_get_pin_type - get pin's type
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 5896b346e579..93901bccf1c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -356,6 +356,7 @@ int ice_read_sma_ctrl(struct ice_hw *hw, u8 *data);
 int ice_write_sma_ctrl(struct ice_hw *hw, u8 data);
 int ice_ptp_read_sdp_ac(struct ice_hw *hw, __le16 *entries, uint *num_entries);
 int ice_cgu_get_num_pins(struct ice_hw *hw, bool input);
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input);
 enum dpll_pin_type ice_cgu_get_pin_type(struct ice_hw *hw, u8 pin, bool input);
 struct dpll_pin_frequency *
 ice_cgu_get_pin_freq_supp(struct ice_hw *hw, u8 pin, bool input, u8 *num);

base-commit: 2fcc88754f4c49e3d9aef226fdfaa1634aa24c66
-- 
2.39.3

Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

Will a notification be provided when the lock is re-acquired?

Another concern is the absence of periodical pin notifications. With the E810, users received the active pin notifications every 1 second. However, the unmanaged DPLL appears to lack this functionality. User implementations currently rely on these periodical notifications to derive the overall clock state, metrics and events from the phase offset. It seems that unmanaged DPLL users will be forced to support two distinct types of DPLLs: one that sends periodical pin notifications and one that does not. Crucially, this difference does not appear to be reflected in the device capabilities, meaning users cannot know in advance whether to expect these notifications.

Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

Will a notification be provided when the lock is re-acquired?

Another concern is the absence of periodical pin notifications. With the E810, users received the active pin notifications every 1 second. However, the unmanaged DPLL appears to lack this functionality. User implementations currently rely on these periodical notifications to derive the overall clock state, metrics and events from the phase offset. It seems that unmanaged DPLL users will be forced to support two distinct types of DPLLs: one that sends periodical pin notifications and one that does not. Crucially, this difference does not appear to be reflected in the device capabilities, meaning users cannot know in advance whether to expect these notifications.

Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

Will a notification be provided when the lock is re-acquired?

Another concern is the absence of periodical pin notifications. With the E810, users received the active pin notifications every 1 second. However, the unmanaged DPLL appears to lack this functionality. User implementations currently rely on these periodical notifications to derive the overall clock state, metrics and events from the phase offset. It seems that unmanaged DPLL users will be forced to support two distinct types of DPLLs: one that sends periodical pin notifications and one that does not. Crucially, this difference does not appear to be reflected in the device capabilities, meaning users cannot know in advance whether to expect these notifications.

RE: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Nitka, Grzegorz]

> -----Original Message-----
> From: Vitaly Grinberg <vgrinber@redhat.com>
> Sent: Tuesday, December 16, 2025 3:42 PM
> To: Nitka, Grzegorz <grzegorz.nitka@intel.com>
> Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nguyen,
> Anthony L <anthony.l.nguyen@intel.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@intel.com>; horms@kernel.org; intel-wired-
> lan@lists.osuosl.org; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> pmenzel@molgen.mpg.de; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Subject: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for
> unmanaged DPLL on E830 NIC
> 
> Will a notification be provided when the lock is re-acquired?
> 

Hi Vitaly, thanks for your comments.
We discussed it offline already, but I think I need more clarifications.

Regarding above question ... yes, 'lock' recovery shall be reported in the same way.
Maybe the name of health status is a little bit misleading (ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK),
However health_info struct contains the current lock status (either 'locked' or 'unlocked').

> Another concern is the absence of periodical pin notifications. With the E810,
> users received the active pin notifications every 1 second. However, the
> unmanaged DPLL appears to lack this functionality. User implementations
> currently rely on these periodical notifications to derive the overall clock
> state, metrics and events from the phase offset. It seems that unmanaged
> DPLL users will be forced to support two distinct types of DPLLs: one that
> sends periodical pin notifications and one that does not. Crucially, this
> difference does not appear to be reflected in the device capabilities,
> meaning users cannot know in advance whether to expect these
> notifications.

After reading it one more time, I'm not sure if I get it right in the first place.
With this patch implementation, there is dpll change notification applied.
By dpll notification I mean calling dpll_device_change_ntf function.
Isn't it what you're looking for?
Notification is triggered only in case when lock status has changed.
It's unmanaged DPLL so the implementation is a little bit simplified, based on FW notification.
There is no need for polling thread like it's done for E810.
But even in case of E810, where polling is applied (2 samples per second), notification is triggered only in case of
dpll/pin status change, not every 1 second.
So please clarify, so either I don't understand the question (please note, I'm only covering the main author)
or notification mechanism, at least about dpll lock state, is already implemented.

Re: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

Hi Grzegors,
Thanks very much for your reply! Added some clarifications inline.

On Wed, Jan 7, 2026 at 11:33 PM Nitka, Grzegorz
<grzegorz.nitka@intel.com> wrote:
>
> > -----Original Message-----
> > From: Vitaly Grinberg <vgrinber@redhat.com>
> > Sent: Tuesday, December 16, 2025 3:42 PM
> > To: Nitka, Grzegorz <grzegorz.nitka@intel.com>
> > Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nguyen,
> > Anthony L <anthony.l.nguyen@intel.com>; Kubalewski, Arkadiusz
> > <arkadiusz.kubalewski@intel.com>; horms@kernel.org; intel-wired-
> > lan@lists.osuosl.org; linux-doc@vger.kernel.org; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org;
> > pmenzel@molgen.mpg.de; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@intel.com>
> > Subject: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for
> > unmanaged DPLL on E830 NIC
> >
> > Will a notification be provided when the lock is re-acquired?
> >
>
> Hi Vitaly, thanks for your comments.
> We discussed it offline already, but I think I need more clarifications.
>
> Regarding above question ... yes, 'lock' recovery shall be reported in the same way.
> Maybe the name of health status is a little bit misleading (ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK),
> However health_info struct contains the current lock status (either 'locked' or 'unlocked').

Great, thanks for clarifying this!

> > Another concern is the absence of periodical pin notifications. With the E810,
> > users received the active pin notifications every 1 second. However, the
> > unmanaged DPLL appears to lack this functionality. User implementations
> > currently rely on these periodical notifications to derive the overall clock
> > state, metrics and events from the phase offset. It seems that unmanaged
> > DPLL users will be forced to support two distinct types of DPLLs: one that
> > sends periodical pin notifications and one that does not. Crucially, this
> > difference does not appear to be reflected in the device capabilities,
> > meaning users cannot know in advance whether to expect these
> > notifications.
>
> After reading it one more time, I'm not sure if I get it right in the first place.
> With this patch implementation, there is dpll change notification applied.
> By dpll notification I mean calling dpll_device_change_ntf function.
> Isn't it what you're looking for?
> Notification is triggered only in case when lock status has changed.
> It's unmanaged DPLL so the implementation is a little bit simplified, based on FW notification.
> There is no need for polling thread like it's done for E810.
> But even in case of E810, where polling is applied (2 samples per second), notification is triggered only in case of
> dpll/pin status change, not every 1 second.
> So please clarify, so either I don't understand the question (please note, I'm only covering the main author)
> or notification mechanism, at least about dpll lock state, is already implemented.
>

Yes, device lock status change notification is definitely what we are
looking for, but there is more. Let me clarify the user perspective.
The e810-based telco system relies on both device and pin
notifications. Phase offset included in pin notifications is critical
because the e810 DPLL "Locked" state is too coarse for Telco
requirements.
It is true that pin notifications are only sent on change; however,
since the phase offset varies slightly with every measurement, the
driver detects a change every second. This effectively turns the
event-driven notification into a periodic one. The e810-based
application strongly relies on the fact that phase offset
notifications are unsolicited and the driver sends them from time to
time.
Now, with the unmanaged DPLL, no pin notification will be sent. Last
time I checked, the device and pin information looked like this:
Device:
 {'clock-id': 1165870453030569040,
  'id': 4,
  'lock-status': 'locked',
  'mode': 'automatic',
  'mode-supported': ['automatic'],
  'module-name': 'ice',
  'type': 'eec'},

Input pin:
{
  "id": 17,
  "module-name": "ice",
  "clock-id": 1165870453030569040,
  "board-label": "1588-TIME_SYNC",
  "type": "ext",
  "capabilities": [],
  "frequency": 10000000,
  "phase-adjust-min": 0,
  "phase-adjust-max": 0,
  "parent-device": [
    {
      "parent-id": 4,
      "state": "connected",
      "direction": "input"
    }
  ]
}

I see a few challenges for the user here. The biggest one is that the
application can't tell the difference between a device that will
report phase offsets and this unmanaged device that never will.
A possible way forward would be adding a capability flag to the DPLL
API so users don't have to guess.
However, the preferred solution would be to simply mirror the E810
behavior (sending phase offset). This preserves the existing API
contract and prevents users, who have already built applications for
this interface, from needing to implement special handling for a new
hardware variant that behaves differently.
There are additional inconsistencies in the existing structure I
wanted to bring to your attention.
1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
device of type "eec". EEC is all about frequency synchronization, and
yet the pin named 1588-TIME_SYNC is clearly a phase pin. This also
doesn't play well with existing implementations, where EEC circuits
deal with frequency, PPS circuits deal with phase, and there is clear
distinction between the two with regard to the meaning of "being
locked".
2. Since it is also an external embedded sync input pin, could it be
possible to expose this information and include `esync-frequency` and
`esync-pulse`? That could be useful for configuring the leading DPLL
that drives the unmanaged one.

RE: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Kubalewski, Arkadiusz]

>From: Vitaly Grinberg <vgrinber@redhat.com>
>Sent: Saturday, January 10, 2026 10:29 PM
>
>Hi Grzegors,
>Thanks very much for your reply! Added some clarifications inline.
>
>On Wed, Jan 7, 2026 at 11:33 PM Nitka, Grzegorz <grzegorz.nitka@intel.com>
>wrote:
>>
>> > -----Original Message-----
>> > From: Vitaly Grinberg <vgrinber@redhat.com>
>> > Sent: Tuesday, December 16, 2025 3:42 PM
>> > To: Nitka, Grzegorz <grzegorz.nitka@intel.com>
>> > Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nguyen,
>> > Anthony L <anthony.l.nguyen@intel.com>; Kubalewski, Arkadiusz
>> > <arkadiusz.kubalewski@intel.com>; horms@kernel.org; intel-wired-
>> > lan@lists.osuosl.org; linux-doc@vger.kernel.org; linux-
>> > kernel@vger.kernel.org; netdev@vger.kernel.org;
>> > pmenzel@molgen.mpg.de; Kitszel, Przemyslaw
>> > <przemyslaw.kitszel@intel.com>
>> > Subject: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support
>> > for unmanaged DPLL on E830 NIC
>> >
>> > Will a notification be provided when the lock is re-acquired?
>> >
>>
>> Hi Vitaly, thanks for your comments.
>> We discussed it offline already, but I think I need more clarifications.
>>
>> Regarding above question ... yes, 'lock' recovery shall be reported in
>>the same way.
>> Maybe the name of health status is a little bit misleading
>> (ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK),
>> However health_info struct contains the current lock status (either
>>'locked' or 'unlocked').
>
>Great, thanks for clarifying this!
>
>> > Another concern is the absence of periodical pin notifications. With
>> > the E810, users received the active pin notifications every 1
>> > second. However, the unmanaged DPLL appears to lack this
>> > functionality. User implementations currently rely on these
>> > periodical notifications to derive the overall clock state, metrics
>> > and events from the phase offset. It seems that unmanaged DPLL users
>> > will be forced to support two distinct types of DPLLs: one that
>> > sends periodical pin notifications and one that does not. Crucially,
>> > this difference does not appear to be reflected in the device
>> > capabilities, meaning users cannot know in advance whether to expect
>> > these notifications.
>>
>> After reading it one more time, I'm not sure if I get it right in the
>> first place.
>> With this patch implementation, there is dpll change notification
>> applied.
>> By dpll notification I mean calling dpll_device_change_ntf function.
>> Isn't it what you're looking for?
>> Notification is triggered only in case when lock status has changed.
>> It's unmanaged DPLL so the implementation is a little bit simplified,
>> based on FW notification.
>> There is no need for polling thread like it's done for E810.
>> But even in case of E810, where polling is applied (2 samples per
>> second), notification is triggered only in case of dpll/pin status
>> change, not every 1 second.
>> So please clarify, so either I don't understand the question (please
>> note, I'm only covering the main author) or notification mechanism, at
>> least about dpll lock state, is already implemented.
>>
>
>Yes, device lock status change notification is definitely what we are
>looking for, but there is more. Let me clarify the user perspective.
>The e810-based telco system relies on both device and pin notifications.
>Phase offset included in pin notifications is critical because the e810
>DPLL "Locked" state is too coarse for Telco requirements.
>It is true that pin notifications are only sent on change; however, since
>the phase offset varies slightly with every measurement, the driver detects
>a change every second. This effectively turns the event-driven notification
>into a periodic one. The e810-based application strongly relies on the fact
>that phase offset notifications are unsolicited and the driver sends them
>from time to time.
>Now, with the unmanaged DPLL, no pin notification will be sent. Last time I
>checked, the device and pin information looked like this:
>Device:
> {'clock-id': 1165870453030569040,
>  'id': 4,
>  'lock-status': 'locked',
>  'mode': 'automatic',
>  'mode-supported': ['automatic'],
>  'module-name': 'ice',
>  'type': 'eec'},
>
>Input pin:
>{
>  "id": 17,
>  "module-name": "ice",
>  "clock-id": 1165870453030569040,
>  "board-label": "1588-TIME_SYNC",
>  "type": "ext",
>  "capabilities": [],
>  "frequency": 10000000,
>  "phase-adjust-min": 0,
>  "phase-adjust-max": 0,
>  "parent-device": [
>    {
>      "parent-id": 4,
>      "state": "connected",
>      "direction": "input"
>    }
>  ]
>}
>
>I see a few challenges for the user here. The biggest one is that the
>application can't tell the difference between a device that will report
>phase offsets and this unmanaged device that never will.
>A possible way forward would be adding a capability flag to the DPLL API so
>users don't have to guess.

There is no phase-offset field as pointed in the above example.
No 'phase-offset' attribute -> no such capability.
Why isn’t that enough?

>However, the preferred solution would be to simply mirror the E810 behavior
>(sending phase offset). This preserves the existing API contract and
>prevents users, who have already built applications for this interface,
>from needing to implement special handling for a new hardware variant that
>behaves differently.

This is not currently possible from driver perspective.
We miss the FW API for it.

>There are additional inconsistencies in the existing structure I wanted to
>bring to your attention.
>1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent device
>of type "eec". EEC is all about frequency synchronization, and yet the pin
>named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play well
>with existing implementations, where EEC circuits deal with frequency, PPS
>circuits deal with phase, and there is clear distinction between the two
>with regard to the meaning of "being locked".

This dpll device type was established based on the main purpose of dpll
device which is to drive the network ports phy clocks with it.

>2. Since it is also an external embedded sync input pin, could it be
>possible to expose this information and include `esync-frequency` and
>`esync-pulse`? That could be useful for configuring the leading DPLL that
>drives the unmanaged one.

Sure, esync caps should be provided, as the commit message example shown:
+    'esync-frequency': 1,
+    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min': 1}],
+    'esync-pulse': 25,

Thank you!
Arkadiusz

Re: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

On Wed, Jan 14, 2026 at 12:23 PM Kubalewski, Arkadiusz
<arkadiusz.kubalewski@intel.com> wrote:
>
> >From: Vitaly Grinberg <vgrinber@redhat.com>
> >Sent: Saturday, January 10, 2026 10:29 PM
> >
> >Hi Grzegors,
> >Thanks very much for your reply! Added some clarifications inline.
> >
> >On Wed, Jan 7, 2026 at 11:33 PM Nitka, Grzegorz <grzegorz.nitka@intel.com>
> >wrote:
> >>
> >> > -----Original Message-----
> >> > From: Vitaly Grinberg <vgrinber@redhat.com>
> >> > Sent: Tuesday, December 16, 2025 3:42 PM
> >> > To: Nitka, Grzegorz <grzegorz.nitka@intel.com>
> >> > Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nguyen,
> >> > Anthony L <anthony.l.nguyen@intel.com>; Kubalewski, Arkadiusz
> >> > <arkadiusz.kubalewski@intel.com>; horms@kernel.org; intel-wired-
> >> > lan@lists.osuosl.org; linux-doc@vger.kernel.org; linux-
> >> > kernel@vger.kernel.org; netdev@vger.kernel.org;
> >> > pmenzel@molgen.mpg.de; Kitszel, Przemyslaw
> >> > <przemyslaw.kitszel@intel.com>
> >> > Subject: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support
> >> > for unmanaged DPLL on E830 NIC
> >> >
> >> > Will a notification be provided when the lock is re-acquired?
> >> >
> >>
> >> Hi Vitaly, thanks for your comments.
> >> We discussed it offline already, but I think I need more clarifications.
> >>
> >> Regarding above question ... yes, 'lock' recovery shall be reported in
> >>the same way.
> >> Maybe the name of health status is a little bit misleading
> >> (ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK),
> >> However health_info struct contains the current lock status (either
> >>'locked' or 'unlocked').
> >
> >Great, thanks for clarifying this!
> >
> >> > Another concern is the absence of periodical pin notifications. With
> >> > the E810, users received the active pin notifications every 1
> >> > second. However, the unmanaged DPLL appears to lack this
> >> > functionality. User implementations currently rely on these
> >> > periodical notifications to derive the overall clock state, metrics
> >> > and events from the phase offset. It seems that unmanaged DPLL users
> >> > will be forced to support two distinct types of DPLLs: one that
> >> > sends periodical pin notifications and one that does not. Crucially,
> >> > this difference does not appear to be reflected in the device
> >> > capabilities, meaning users cannot know in advance whether to expect
> >> > these notifications.
> >>
> >> After reading it one more time, I'm not sure if I get it right in the
> >> first place.
> >> With this patch implementation, there is dpll change notification
> >> applied.
> >> By dpll notification I mean calling dpll_device_change_ntf function.
> >> Isn't it what you're looking for?
> >> Notification is triggered only in case when lock status has changed.
> >> It's unmanaged DPLL so the implementation is a little bit simplified,
> >> based on FW notification.
> >> There is no need for polling thread like it's done for E810.
> >> But even in case of E810, where polling is applied (2 samples per
> >> second), notification is triggered only in case of dpll/pin status
> >> change, not every 1 second.
> >> So please clarify, so either I don't understand the question (please
> >> note, I'm only covering the main author) or notification mechanism, at
> >> least about dpll lock state, is already implemented.
> >>
> >
> >Yes, device lock status change notification is definitely what we are
> >looking for, but there is more. Let me clarify the user perspective.
> >The e810-based telco system relies on both device and pin notifications.
> >Phase offset included in pin notifications is critical because the e810
> >DPLL "Locked" state is too coarse for Telco requirements.
> >It is true that pin notifications are only sent on change; however, since
> >the phase offset varies slightly with every measurement, the driver detects
> >a change every second. This effectively turns the event-driven notification
> >into a periodic one. The e810-based application strongly relies on the fact
> >that phase offset notifications are unsolicited and the driver sends them
> >from time to time.
> >Now, with the unmanaged DPLL, no pin notification will be sent. Last time I
> >checked, the device and pin information looked like this:
> >Device:
> > {'clock-id': 1165870453030569040,
> >  'id': 4,
> >  'lock-status': 'locked',
> >  'mode': 'automatic',
> >  'mode-supported': ['automatic'],
> >  'module-name': 'ice',
> >  'type': 'eec'},
> >
> >Input pin:
> >{
> >  "id": 17,
> >  "module-name": "ice",
> >  "clock-id": 1165870453030569040,
> >  "board-label": "1588-TIME_SYNC",
> >  "type": "ext",
> >  "capabilities": [],
> >  "frequency": 10000000,
> >  "phase-adjust-min": 0,
> >  "phase-adjust-max": 0,
> >  "parent-device": [
> >    {
> >      "parent-id": 4,
> >      "state": "connected",
> >      "direction": "input"
> >    }
> >  ]
> >}
> >
> >I see a few challenges for the user here. The biggest one is that the
> >application can't tell the difference between a device that will report
> >phase offsets and this unmanaged device that never will.
> >A possible way forward would be adding a capability flag to the DPLL API so
> >users don't have to guess.
>
> There is no phase-offset field as pointed in the above example.
> No 'phase-offset' attribute -> no such capability.
> Why isn’t that enough?

Pin reply does not contain phase offset, so no change notifications
are expected?
Could there be devices that don't report phase offset, but report state changes?
Is this the intended use of the phase offset API to be interpreted as
a general pin
notification capability flag?

>
> >However, the preferred solution would be to simply mirror the E810 behavior
> >(sending phase offset). This preserves the existing API contract and
> >prevents users, who have already built applications for this interface,
> >from needing to implement special handling for a new hardware variant that
> >behaves differently.
>
> This is not currently possible from driver perspective.
> We miss the FW API for it.
>
> >There are additional inconsistencies in the existing structure I wanted to
> >bring to your attention.
> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent device
> >of type "eec". EEC is all about frequency synchronization, and yet the pin
> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play well
> >with existing implementations, where EEC circuits deal with frequency, PPS
> >circuits deal with phase, and there is clear distinction between the two
> >with regard to the meaning of "being locked".
>
> This dpll device type was established based on the main purpose of dpll
> device which is to drive the network ports phy clocks with it.

What is the physical meaning of this indication (lock-status':
'locked')? Locked on what?
As a user of this circuit I want to know that the device is locked on
the phase of the input signal with a certain precision.
Is this the meaning of "locked" here? Can an EEC device be locked on
the Phase of the input signal?
Users of other devices (e810, zl3073x) may have implemented logic to
determine the phase lock by
enforcing the pin parent device type as PPS. How should they change it
to determine phase lock (and why)?

>
> >2. Since it is also an external embedded sync input pin, could it be
> >possible to expose this information and include `esync-frequency` and
> >`esync-pulse`? That could be useful for configuring the leading DPLL that
> >drives the unmanaged one.
>
> Sure, esync caps should be provided, as the commit message example shown:
> +    'esync-frequency': 1,
> +    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min': 1}],
> +    'esync-pulse': 25,
>

Oh, I must have missed that.
Thanks!
Vitaly

> Thank you!
> Arkadiusz

RE: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Kubalewski, Arkadiusz]

>From: Vitaly Grinberg <vgrinber@redhat.com>
>Sent: Wednesday, January 14, 2026 12:39 PM
>

[..]

>> >
>> >I see a few challenges for the user here. The biggest one is that the
>> >application can't tell the difference between a device that will report
>> >phase offsets and this unmanaged device that never will.
>> >A possible way forward would be adding a capability flag to the DPLL API
>> >so
>> >users don't have to guess.
>>
>> There is no phase-offset field as pointed in the above example.
>> No 'phase-offset' attribute -> no such capability.
>> Why isn’t that enough?
>
>Pin reply does not contain phase offset, so no change notifications
>are expected?
>Could there be devices that don't report phase offset, but report state
>changes?
>Is this the intended use of the phase offset API to be interpreted as
>a general pin
>notification capability flag?
>

Sorry, this is not what I meant.

The E810 produces notifications not only for the pin's phase offset but
also for other pin attribute changes. When it comes to the E810 pins,
notifications generated by phase offset changes are quite frequent.
However, it wasn't intention to produce them every second; this is simply
the result of frequent phase offset changes.

Typically, the pin state changes for the pin, but for E830, the unmanaged
mode means that the state of the pin never changes, resulting in no pin
notifications being produced in the end.

Hope that clears things up.

>>
>> >However, the preferred solution would be to simply mirror the E810
>> >behavior
>> >(sending phase offset). This preserves the existing API contract and
>> >prevents users, who have already built applications for this interface,
>> >from needing to implement special handling for a new hardware variant
>> >that
>> >behaves differently.
>>
>> This is not currently possible from driver perspective.
>> We miss the FW API for it.
>>
>> >There are additional inconsistencies in the existing structure I wanted
>> >to
>> >bring to your attention.
>> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
>> >device
>> >of type "eec". EEC is all about frequency synchronization, and yet the
>> >pin
>> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play well
>> >with existing implementations, where EEC circuits deal with frequency,
>> >PPS
>> >circuits deal with phase, and there is clear distinction between the two
>> >with regard to the meaning of "being locked".
>>
>> This dpll device type was established based on the main purpose of dpll
>> device which is to drive the network ports phy clocks with it.
>
>What is the physical meaning of this indication (lock-status':
>'locked')? Locked on what?

Lock status is dpll device property.

But full picture has to be determined from the list of pins, for this
particular case, one input provided from host through pci-e pin, 10MHz
bandwidth frequency and 1 PPS sync pulses.

As already pointed the type of dpll shall let user know the purpose of
the dpll existence instead of particular input properties.
Input properties are determined with the pin's attributes.

>As a user of this circuit I want to know that the device is locked on
>the phase of the input signal with a certain precision.
>Is this the meaning of "locked" here? Can an EEC device be locked on
>the Phase of the input signal?

Well I don't have any data on the precision of such, but AFAIK it can.
EEC dpll shall be producing stable signal, the input it uses is only
part of the full dpll device picture.

>Users of other devices (e810, zl3073x) may have implemented logic to
>determine the phase lock by
>enforcing the pin parent device type as PPS. How should they change it
>to determine phase lock (and why)?
>

I am Sorry, I don't understand the example above, could you please
Elaborate on details of such setup?

Thank you!
Arkadiusz

>>
>> >2. Since it is also an external embedded sync input pin, could it be
>> >possible to expose this information and include `esync-frequency` and
>> >`esync-pulse`? That could be useful for configuring the leading DPLL
>> >that
>> >drives the unmanaged one.
>>
>> Sure, esync caps should be provided, as the commit message example shown:
>> +    'esync-frequency': 1,
>> +    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min':
>>1}],
>> +    'esync-pulse': 25,
>>
>
>Oh, I must have missed that.
>Thanks!
>Vitaly
>
>> Thank you!
>> Arkadiusz

Re: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

, the ea

On Wed, Jan 14, 2026 at 3:32 PM Kubalewski, Arkadiusz
<arkadiusz.kubalewski@intel.com> wrote:
>
> >From: Vitaly Grinberg <vgrinber@redhat.com>
> >Sent: Wednesday, January 14, 2026 12:39 PM
> >
>
> [..]
>
> >> >
> >> >I see a few challenges for the user here. The biggest one is that the
> >> >application can't tell the difference between a device that will report
> >> >phase offsets and this unmanaged device that never will.
> >> >A possible way forward would be adding a capability flag to the DPLL API
> >> >so
> >> >users don't have to guess.
> >>
> >> There is no phase-offset field as pointed in the above example.
> >> No 'phase-offset' attribute -> no such capability.
> >> Why isn’t that enough?
> >
> >Pin reply does not contain phase offset, so no change notifications
> >are expected?
> >Could there be devices that don't report phase offset, but report state
> >changes?
> >Is this the intended use of the phase offset API to be interpreted as
> >a general pin
> >notification capability flag?
> >
>
> Sorry, this is not what I meant.
>
> The E810 produces notifications not only for the pin's phase offset but
> also for other pin attribute changes. When it comes to the E810 pins,
> notifications generated by phase offset changes are quite frequent.
> However, it wasn't intention to produce them every second; this is simply
> the result of frequent phase offset changes.
>
> Typically, the pin state changes for the pin, but for E830, the unmanaged
> mode means that the state of the pin never changes, resulting in no pin
> notifications being produced in the end.
>
> Hope that clears things up.

Will the reported pin state remain "connected" even if I disconnect
the input signal?
Is there any information in DPLL or pin replies that can tell the user
"this DPLL is unmanaged type, it behaves differently"?

>
> >>
> >> >However, the preferred solution would be to simply mirror the E810
> >> >behavior
> >> >(sending phase offset). This preserves the existing API contract and
> >> >prevents users, who have already built applications for this interface,
> >> >from needing to implement special handling for a new hardware variant
> >> >that
> >> >behaves differently.
> >>
> >> This is not currently possible from driver perspective.
> >> We miss the FW API for it.
> >>
> >> >There are additional inconsistencies in the existing structure I wanted
> >> >to
> >> >bring to your attention.
> >> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
> >> >device
> >> >of type "eec". EEC is all about frequency synchronization, and yet the
> >> >pin
> >> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play well
> >> >with existing implementations, where EEC circuits deal with frequency,
> >> >PPS
> >> >circuits deal with phase, and there is clear distinction between the two
> >> >with regard to the meaning of "being locked".
> >>
> >> This dpll device type was established based on the main purpose of dpll
> >> device which is to drive the network ports phy clocks with it.
> >
> >What is the physical meaning of this indication (lock-status':
> >'locked')? Locked on what?
>
> Lock status is dpll device property.
>
> But full picture has to be determined from the list of pins, for this
> particular case, one input provided from host through pci-e pin, 10MHz
> bandwidth frequency and 1 PPS sync pulses.
>
> As already pointed the type of dpll shall let user know the purpose of
> the dpll existence instead of particular input properties.
> Input properties are determined with the pin's attributes.
>
> >As a user of this circuit I want to know that the device is locked on
> >the phase of the input signal with a certain precision.
> >Is this the meaning of "locked" here? Can an EEC device be locked on
> >the Phase of the input signal?
>
> Well I don't have any data on the precision of such, but AFAIK it can.
> EEC dpll shall be producing stable signal, the input it uses is only
> part of the full dpll device picture.
>
> >Users of other devices (e810, zl3073x) may have implemented logic to
> >determine the phase lock by
> >enforcing the pin parent device type as PPS. How should they change it
> >to determine phase lock (and why)?
> >
>
> I am Sorry, I don't understand the example above, could you please
> Elaborate on details of such setup?

Yep, gladly!
Telco customers rely on the Phase being locked on the reference with a
certain precision. In E810 and zl3073x the equation is simple:
1. "eec locked" means synTonization achieved - frequency locked
2. "pps locked" means synChronization achieved - phase locked
The T-BC application checks the reported device type. If the device
type is "pps", the report is processed by the synchronization state
decision logic. Otherwise, the report doesn't have any meaning within
"T-BC without SyncE" context and is discarded.

Since this patch is going to create eec reports only, they are all
going to be discarded, and this is a compatibility issue I'm trying to
address.

Could you please answer my question above:
What is the physical meaning of this indication
(lock-status':'locked') in this report?

Thanks!
VG

> Thank you!
> Arkadiusz
>
> >>
> >> >2. Since it is also an external embedded sync input pin, could it be
> >> >possible to expose this information and include `esync-frequency` and
> >> >`esync-pulse`? That could be useful for configuring the leading DPLL
> >> >that
> >> >drives the unmanaged one.
> >>
> >> Sure, esync caps should be provided, as the commit message example shown:
> >> +    'esync-frequency': 1,
> >> +    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min':
> >>1}],
> >> +    'esync-pulse': 25,
> >>
> >
> >Oh, I must have missed that.
> >Thanks!
> >Vitaly
> >
> >> Thank you!
> >> Arkadiusz
>

RE: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Kubalewski, Arkadiusz]

>From: Vitaly Grinberg <vgrinber@redhat.com>
>Sent: Thursday, January 15, 2026 9:58 AM
>
>, the ea
>
>On Wed, Jan 14, 2026 at 3:32 PM Kubalewski, Arkadiusz
><arkadiusz.kubalewski@intel.com> wrote:
>>
>> >From: Vitaly Grinberg <vgrinber@redhat.com>
>> >Sent: Wednesday, January 14, 2026 12:39 PM
>> >
>>
>> [..]
>>
>> >> >
>> >> >I see a few challenges for the user here. The biggest one is that
>> >> >the
>> >> >application can't tell the difference between a device that will
>> >> >report
>> >> >phase offsets and this unmanaged device that never will.
>> >> >A possible way forward would be adding a capability flag to the DPLL
>> >> >API
>> >> >so
>> >> >users don't have to guess.
>> >>
>> >> There is no phase-offset field as pointed in the above example.
>> >> No 'phase-offset' attribute -> no such capability.
>> >> Why isn’t that enough?
>> >
>> >Pin reply does not contain phase offset, so no change notifications
>> >are expected?
>> >Could there be devices that don't report phase offset, but report state
>> >changes?
>> >Is this the intended use of the phase offset API to be interpreted as
>> >a general pin
>> >notification capability flag?
>> >
>>
>> Sorry, this is not what I meant.
>>
>> The E810 produces notifications not only for the pin's phase offset but
>> also for other pin attribute changes. When it comes to the E810 pins,
>> notifications generated by phase offset changes are quite frequent.
>> However, it wasn't intention to produce them every second; this is
>simply
>> the result of frequent phase offset changes.
>>
>> Typically, the pin state changes for the pin, but for E830, the
>> unmanaged
>> mode means that the state of the pin never changes, resulting in no pin
>> notifications being produced in the end.
>>
>> Hope that clears things up.
>
>Will the reported pin state remain "connected" even if I disconnect
>the input signal?
>Is there any information in DPLL or pin replies that can tell the user
>"this DPLL is unmanaged type, it behaves differently"?

You really cannot disconnect anything there, it is always connected,
and no one can change it. It only shows the user actual physical
connections hardwired into the NIC/dpll. There is no SW handled config
possible on those. As provided in the commit message, the pins have empty
capabilities set: 'capabilities': set(), thus not possible to change state
by the user.

>
>>
>> >>
>> >> >However, the preferred solution would be to simply mirror the E810
>> >> >behavior
>> >> >(sending phase offset). This preserves the existing API contract and
>> >> >prevents users, who have already built applications for this
>> >> >interface,
>> >> >from needing to implement special handling for a new hardware
>> >> >variant
>> >> >that
>> >> >behaves differently.
>> >>
>> >> This is not currently possible from driver perspective.
>> >> We miss the FW API for it.
>> >>
>> >> >There are additional inconsistencies in the existing structure I
>> >> >wanted
>> >> >to
>> >> >bring to your attention.
>> >> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
>> >> >device
>> >> >of type "eec". EEC is all about frequency synchronization, and yet
>> >> >the
>> >> >pin
>> >> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play
>> >> >well
>> >> >with existing implementations, where EEC circuits deal with
>> >> >frequency,
>> >> >PPS
>> >> >circuits deal with phase, and there is clear distinction between the
>> >> >two
>> >> >with regard to the meaning of "being locked".
>> >>
>> >> This dpll device type was established based on the main purpose of
>> >> >dpll
>> >> device which is to drive the network ports phy clocks with it.
>> >
>> >What is the physical meaning of this indication (lock-status':
>> >'locked')? Locked on what?
>>
>> Lock status is dpll device property.
>>
>> But full picture has to be determined from the list of pins, for this
>> particular case, one input provided from host through pci-e pin, 10MHz
>> bandwidth frequency and 1 PPS sync pulses.
>>
>> As already pointed the type of dpll shall let user know the purpose of
>> the dpll existence instead of particular input properties.
>> Input properties are determined with the pin's attributes.
>>
>> >As a user of this circuit I want to know that the device is locked on
>> >the phase of the input signal with a certain precision.
>> >Is this the meaning of "locked" here? Can an EEC device be locked on
>> >the Phase of the input signal?
>>
>> Well I don't have any data on the precision of such, but AFAIK it can.
>> EEC dpll shall be producing stable signal, the input it uses is only
>> part of the full dpll device picture.
>>
>> >Users of other devices (e810, zl3073x) may have implemented logic to
>> >determine the phase lock by
>> >enforcing the pin parent device type as PPS. How should they change it
>> >to determine phase lock (and why)?
>> >
>>
>> I am Sorry, I don't understand the example above, could you please
>> Elaborate on details of such setup?
>
>Yep, gladly!
>Telco customers rely on the Phase being locked on the reference with a
>certain precision. In E810 and zl3073x the equation is simple:
>1. "eec locked" means synTonization achieved - frequency locked
>2. "pps locked" means synChronization achieved - phase locked
>The T-BC application checks the reported device type. If the device
>type is "pps", the report is processed by the synchronization state
>decision logic. Otherwise, the report doesn't have any meaning within
>"T-BC without SyncE" context and is discarded.
>
>Since this patch is going to create eec reports only, they are all
>going to be discarded, and this is a compatibility issue I'm trying to
>address.
>

I see. From this angle the PPS type looks like a superset of EEC.
It makes sense to me. We had also some discussion and agreed to propose
new patch with the PPS type, as we don't want the underlying SW to be
troubled with such issue.

>Could you please answer my question above:
>What is the physical meaning of this indication
>(lock-status':'locked') in this report?
>

It means that dpll is locked/synchronized with the esync 10MHz/1PPS,
a signal provided through pcie pin.

Thank you!
Arkadiusz

>Thanks!
>VG
>
>> Thank you!
>> Arkadiusz
>>
>> >>
>> >> >2. Since it is also an external embedded sync input pin, could it be
>> >> >possible to expose this information and include `esync-frequency`
>> >> >and
>> >> >`esync-pulse`? That could be useful for configuring the leading DPLL
>> >> >that
>> >> >drives the unmanaged one.
>> >>
>> >> Sure, esync caps should be provided, as the commit message example
>> >> >shown:
>> >> +    'esync-frequency': 1,
>> >> +    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-
>> >> >min':
>> >>1}],
>> >> +    'esync-pulse': 25,
>> >>
>> >
>> >Oh, I must have missed that.
>> >Thanks!
>> >Vitaly
>> >
>> >> Thank you!
>> >> Arkadiusz
>>

Re: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Vitaly Grinberg]

On Thu, Jan 15, 2026 at 5:30 PM Kubalewski, Arkadiusz
<arkadiusz.kubalewski@intel.com> wrote:
>
> >From: Vitaly Grinberg <vgrinber@redhat.com>
> >Sent: Thursday, January 15, 2026 9:58 AM
> >
> >, the ea
> >
> >On Wed, Jan 14, 2026 at 3:32 PM Kubalewski, Arkadiusz
> ><arkadiusz.kubalewski@intel.com> wrote:
> >>
> >> >From: Vitaly Grinberg <vgrinber@redhat.com>
> >> >Sent: Wednesday, January 14, 2026 12:39 PM
> >> >
> >>
> >> [..]
> >>
> >> >> >
> >> >> >I see a few challenges for the user here. The biggest one is that
> >> >> >the
> >> >> >application can't tell the difference between a device that will
> >> >> >report
> >> >> >phase offsets and this unmanaged device that never will.
> >> >> >A possible way forward would be adding a capability flag to the DPLL
> >> >> >API
> >> >> >so
> >> >> >users don't have to guess.
> >> >>
> >> >> There is no phase-offset field as pointed in the above example.
> >> >> No 'phase-offset' attribute -> no such capability.
> >> >> Why isn’t that enough?
> >> >
> >> >Pin reply does not contain phase offset, so no change notifications
> >> >are expected?
> >> >Could there be devices that don't report phase offset, but report state
> >> >changes?
> >> >Is this the intended use of the phase offset API to be interpreted as
> >> >a general pin
> >> >notification capability flag?
> >> >
> >>
> >> Sorry, this is not what I meant.
> >>
> >> The E810 produces notifications not only for the pin's phase offset but
> >> also for other pin attribute changes. When it comes to the E810 pins,
> >> notifications generated by phase offset changes are quite frequent.
> >> However, it wasn't intention to produce them every second; this is
> >simply
> >> the result of frequent phase offset changes.
> >>
> >> Typically, the pin state changes for the pin, but for E830, the
> >> unmanaged
> >> mode means that the state of the pin never changes, resulting in no pin
> >> notifications being produced in the end.
> >>
> >> Hope that clears things up.
> >
> >Will the reported pin state remain "connected" even if I disconnect
> >the input signal?
> >Is there any information in DPLL or pin replies that can tell the user
> >"this DPLL is unmanaged type, it behaves differently"?
>
> You really cannot disconnect anything there, it is always connected,
> and no one can change it. It only shows the user actual physical
> connections hardwired into the NIC/dpll. There is no SW handled config
> possible on those. As provided in the commit message, the pins have empty
> capabilities set: 'capabilities': set(), thus not possible to change state
> by the user.
>

Got it, thanks for clarification!

> >
> >>
> >> >>
> >> >> >However, the preferred solution would be to simply mirror the E810
> >> >> >behavior
> >> >> >(sending phase offset). This preserves the existing API contract and
> >> >> >prevents users, who have already built applications for this
> >> >> >interface,
> >> >> >from needing to implement special handling for a new hardware
> >> >> >variant
> >> >> >that
> >> >> >behaves differently.
> >> >>
> >> >> This is not currently possible from driver perspective.
> >> >> We miss the FW API for it.
> >> >>
> >> >> >There are additional inconsistencies in the existing structure I
> >> >> >wanted
> >> >> >to
> >> >> >bring to your attention.
> >> >> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
> >> >> >device
> >> >> >of type "eec". EEC is all about frequency synchronization, and yet
> >> >> >the
> >> >> >pin
> >> >> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't play
> >> >> >well
> >> >> >with existing implementations, where EEC circuits deal with
> >> >> >frequency,
> >> >> >PPS
> >> >> >circuits deal with phase, and there is clear distinction between the
> >> >> >two
> >> >> >with regard to the meaning of "being locked".
> >> >>
> >> >> This dpll device type was established based on the main purpose of
> >> >> >dpll
> >> >> device which is to drive the network ports phy clocks with it.
> >> >
> >> >What is the physical meaning of this indication (lock-status':
> >> >'locked')? Locked on what?
> >>
> >> Lock status is dpll device property.
> >>
> >> But full picture has to be determined from the list of pins, for this
> >> particular case, one input provided from host through pci-e pin, 10MHz
> >> bandwidth frequency and 1 PPS sync pulses.
> >>
> >> As already pointed the type of dpll shall let user know the purpose of
> >> the dpll existence instead of particular input properties.
> >> Input properties are determined with the pin's attributes.
> >>
> >> >As a user of this circuit I want to know that the device is locked on
> >> >the phase of the input signal with a certain precision.
> >> >Is this the meaning of "locked" here? Can an EEC device be locked on
> >> >the Phase of the input signal?
> >>
> >> Well I don't have any data on the precision of such, but AFAIK it can.
> >> EEC dpll shall be producing stable signal, the input it uses is only
> >> part of the full dpll device picture.
> >>
> >> >Users of other devices (e810, zl3073x) may have implemented logic to
> >> >determine the phase lock by
> >> >enforcing the pin parent device type as PPS. How should they change it
> >> >to determine phase lock (and why)?
> >> >
> >>
> >> I am Sorry, I don't understand the example above, could you please
> >> Elaborate on details of such setup?
> >
> >Yep, gladly!
> >Telco customers rely on the Phase being locked on the reference with a
> >certain precision. In E810 and zl3073x the equation is simple:
> >1. "eec locked" means synTonization achieved - frequency locked
> >2. "pps locked" means synChronization achieved - phase locked
> >The T-BC application checks the reported device type. If the device
> >type is "pps", the report is processed by the synchronization state
> >decision logic. Otherwise, the report doesn't have any meaning within
> >"T-BC without SyncE" context and is discarded.
> >
> >Since this patch is going to create eec reports only, they are all
> >going to be discarded, and this is a compatibility issue I'm trying to
> >address.
> >
>
> I see. From this angle the PPS type looks like a superset of EEC.
> It makes sense to me. We had also some discussion and agreed to propose
> new patch with the PPS type, as we don't want the underlying SW to be
> troubled with such issue.

Thanks very much for considering this!

> >Could you please answer my question above:
> >What is the physical meaning of this indication
> >(lock-status':'locked') in this report?
> >
>
> It means that dpll is locked/synchronized with the esync 10MHz/1PPS,
> a signal provided through pcie pin.
>
> Thank you!
> Arkadiusz
>
> >Thanks!
> >VG
> >
> >> Thank you!
> >> Arkadiusz
> >>
> >> >>
> >> >> >2. Since it is also an external embedded sync input pin, could it be
> >> >> >possible to expose this information and include `esync-frequency`
> >> >> >and
> >> >> >`esync-pulse`? That could be useful for configuring the leading DPLL
> >> >> >that
> >> >> >drives the unmanaged one.
> >> >>
> >> >> Sure, esync caps should be provided, as the commit message example
> >> >> >shown:
> >> >> +    'esync-frequency': 1,
> >> >> +    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-
> >> >> >min':
> >> >>1}],
> >> >> +    'esync-pulse': 25,
> >> >>
> >> >
> >> >Oh, I must have missed that.
> >> >Thanks!
> >> >Vitaly
> >> >
> >> >> Thank you!
> >> >> Arkadiusz
> >>
>

RE: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for unmanaged DPLL on E830 NIC

[Kubalewski, Arkadiusz]

>From: Vitaly Grinberg <vgrinber@redhat.com>
>Sent: Thursday, January 15, 2026 6:26 PM
>
>On Thu, Jan 15, 2026 at 5:30 PM Kubalewski, Arkadiusz
><arkadiusz.kubalewski@intel.com> wrote:
>>
>> >From: Vitaly Grinberg <vgrinber@redhat.com>
>> >Sent: Thursday, January 15, 2026 9:58 AM
>> >
>> >, the ea
>> >
>> >On Wed, Jan 14, 2026 at 3:32 PM Kubalewski, Arkadiusz
>> ><arkadiusz.kubalewski@intel.com> wrote:
>> >>
>> >> >From: Vitaly Grinberg <vgrinber@redhat.com>
>> >> >Sent: Wednesday, January 14, 2026 12:39 PM
>> >> >
>> >>
>> >> [..]
>> >>
>> >> >> >
>> >> >> >I see a few challenges for the user here. The biggest one is that
>> >> >> >the
>> >> >> >application can't tell the difference between a device that will
>> >> >> >report
>> >> >> >phase offsets and this unmanaged device that never will.
>> >> >> >A possible way forward would be adding a capability flag to the
>> >> >> >DPLL
>> >> >> >API
>> >> >> >so
>> >> >> >users don't have to guess.
>> >> >>
>> >> >> There is no phase-offset field as pointed in the above example.
>> >> >> No 'phase-offset' attribute -> no such capability.
>> >> >> Why isn’t that enough?
>> >> >
>> >> >Pin reply does not contain phase offset, so no change notifications
>> >> >are expected?
>> >> >Could there be devices that don't report phase offset, but report
>> >> >state
>> >> >changes?
>> >> >Is this the intended use of the phase offset API to be interpreted
>> >> >as
>> >> >a general pin
>> >> >notification capability flag?
>> >> >
>> >>
>> >> Sorry, this is not what I meant.
>> >>
>> >> The E810 produces notifications not only for the pin's phase offset
>> >> but
>> >> also for other pin attribute changes. When it comes to the E810 pins,
>> >> notifications generated by phase offset changes are quite frequent.
>> >> However, it wasn't intention to produce them every second; this is
>> >> simply
>> >> the result of frequent phase offset changes.
>> >>
>> >> Typically, the pin state changes for the pin, but for E830, the
>> >> unmanaged
>> >> mode means that the state of the pin never changes, resulting in no
>> >> pin
>> >> notifications being produced in the end.
>> >>
>> >> Hope that clears things up.
>> >
>> >Will the reported pin state remain "connected" even if I disconnect
>> >the input signal?
>> >Is there any information in DPLL or pin replies that can tell the user
>> >"this DPLL is unmanaged type, it behaves differently"?
>>
>> You really cannot disconnect anything there, it is always connected,
>> and no one can change it. It only shows the user actual physical
>> connections hardwired into the NIC/dpll. There is no SW handled config
>> possible on those. As provided in the commit message, the pins have
>> empty
>> capabilities set: 'capabilities': set(), thus not possible to change
>> state
>> by the user.
>>
>
>Got it, thanks for clarification!
>

I am glad I could help.

>> >
>> >>
>> >> >>
>> >> >> >However, the preferred solution would be to simply mirror the
>> >> >> >E810
>> >> >> >behavior
>> >> >> >(sending phase offset). This preserves the existing API contract
>> >> >> >and
>> >> >> >prevents users, who have already built applications for this
>> >> >> >interface,
>> >> >> >from needing to implement special handling for a new hardware
>> >> >> >variant
>> >> >> >that
>> >> >> >behaves differently.
>> >> >>
>> >> >> This is not currently possible from driver perspective.
>> >> >> We miss the FW API for it.
>> >> >>
>> >> >> >There are additional inconsistencies in the existing structure I
>> >> >> >wanted
>> >> >> >to
>> >> >> >bring to your attention.
>> >> >> >1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a
>> >> >> >parent
>> >> >> >device
>> >> >> >of type "eec". EEC is all about frequency synchronization, and
>> >> >> >yet
>> >> >> >the
>> >> >> >pin
>> >> >> >named 1588-TIME_SYNC is clearly a phase pin. This also doesn't
>> >> >> >play
>> >> >> >well
>> >> >> >with existing implementations, where EEC circuits deal with
>> >> >> >frequency,
>> >> >> >PPS
>> >> >> >circuits deal with phase, and there is clear distinction between
>> >> >> >the
>> >> >> >two
>> >> >> >with regard to the meaning of "being locked".
>> >> >>
>> >> >> This dpll device type was established based on the main purpose of
>> >> >> >dpll
>> >> >> device which is to drive the network ports phy clocks with it.
>> >> >
>> >> >What is the physical meaning of this indication (lock-status':
>> >> >'locked')? Locked on what?
>> >>
>> >> Lock status is dpll device property.
>> >>
>> >> But full picture has to be determined from the list of pins, for this
>> >> particular case, one input provided from host through pci-e pin,
>> >> 10MHz
>> >> bandwidth frequency and 1 PPS sync pulses.
>> >>
>> >> As already pointed the type of dpll shall let user know the purpose
>> >> of
>> >> the dpll existence instead of particular input properties.
>> >> Input properties are determined with the pin's attributes.
>> >>
>> >> >As a user of this circuit I want to know that the device is locked
>> >> >on
>> >> >the phase of the input signal with a certain precision.
>> >> >Is this the meaning of "locked" here? Can an EEC device be locked on
>> >> >the Phase of the input signal?
>> >>
>> >> Well I don't have any data on the precision of such, but AFAIK it
>> >> can.
>> >> EEC dpll shall be producing stable signal, the input it uses is only
>> >> part of the full dpll device picture.
>> >>
>> >> >Users of other devices (e810, zl3073x) may have implemented logic to
>> >> >determine the phase lock by
>> >> >enforcing the pin parent device type as PPS. How should they change
>> >> >it
>> >> >to determine phase lock (and why)?
>> >> >
>> >>
>> >> I am Sorry, I don't understand the example above, could you please
>> >> Elaborate on details of such setup?
>> >
>> >Yep, gladly!
>> >Telco customers rely on the Phase being locked on the reference with a
>> >certain precision. In E810 and zl3073x the equation is simple:
>> >1. "eec locked" means synTonization achieved - frequency locked
>> >2. "pps locked" means synChronization achieved - phase locked
>> >The T-BC application checks the reported device type. If the device
>> >type is "pps", the report is processed by the synchronization state
>> >decision logic. Otherwise, the report doesn't have any meaning within
>> >"T-BC without SyncE" context and is discarded.
>> >
>> >Since this patch is going to create eec reports only, they are all
>> >going to be discarded, and this is a compatibility issue I'm trying to
>> >address.
>> >
>>
>> I see. From this angle the PPS type looks like a superset of EEC.
>> It makes sense to me. We had also some discussion and agreed to propose
>> new patch with the PPS type, as we don't want the underlying SW to be
>> troubled with such issue.
>
>Thanks very much for considering this!

Sure, v6 is there, please have a look.

Thank you!
Arkadiusz

[..]