All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-10 13:45 ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: maciej.machnikowski, netdev, intel-wired-lan, arkadiusz.kubalewski
  Cc: richardcochran, abyagowi, anthony.l.nguyen, davem, kuba,
	linux-kselftest, idosch, mkubecek, saeed, michael.chan, petrm

Synchronous Ethernet networks use a physical layer clock to syntonize
the frequency across different network elements.

Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
Equipment Clock (EEC) and have the ability to synchronize to reference
frequency sources.

This patch series is a prerequisite for EEC object and adds ability
to enable recovered clocks in the physical layer of the netdev object.
Recovered clocks can be used as one of the reference signal by the EEC.

Further work is required to add the DPLL subsystem, link it to the
netdev object and create API to read the EEC DPLL state.

v5:
- rewritten the documentation
- fixed doxygen headers

v4:
- Dropped EEC_STATE reporting (TBD: DPLL subsystem)
- moved recovered clock configuration to ethtool netlink

v3:
- remove RTM_GETRCLKRANGE
- return state of all possible pins in the RTM_GETRCLKSTATE
- clarify documentation

v2:
- improved documentation
- fixed kdoc warning

RFC history:
v2:
- removed whitespace changes
- fix issues reported by test robot
v3:
- Changed naming from SyncE to EEC
- Clarify cover letter and commit message for patch 1
v4:
- Removed sync_source and pin_idx info
- Changed one structure to attributes
- Added EEC_SRC_PORT flag to indicate that the EEC is synchronized
  to the recovered clock of a port that returns the state
v5:
- add EEC source as an optiona attribute
- implement support for recovered clocks
- align states returned by EEC to ITU-T G.781
v6:
- fix EEC clock state reporting
- add documentation
- fix descriptions in code comments


Maciej Machnikowski (4):
  ice: add support detecting features based on netlist
  ethtool: Add ability to configure recovered clock for SyncE feature
  ice: add support for monitoring SyncE DPLL state
  ice: add support for recovered clocks

 Documentation/networking/ethtool-netlink.rst  |  62 ++++
 drivers/net/ethernet/intel/ice/ice.h          |   7 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  70 ++++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 224 +++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  20 +-
 drivers/net/ethernet/intel/ice/ice_devids.h   |   3 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |  96 +++++++
 drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  35 +++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  49 ++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  36 +++
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 include/linux/ethtool.h                       |   9 +
 include/uapi/linux/ethtool_netlink.h          |  21 ++
 net/ethtool/Makefile                          |   3 +-
 net/ethtool/netlink.c                         |  20 ++
 net/ethtool/netlink.h                         |   4 +
 net/ethtool/synce.c                           | 267 ++++++++++++++++++
 18 files changed, 929 insertions(+), 4 deletions(-)
 create mode 100644 net/ethtool/synce.c

-- 
2.26.3


^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-10 13:45 ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: intel-wired-lan

Synchronous Ethernet networks use a physical layer clock to syntonize
the frequency across different network elements.

Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
Equipment Clock (EEC) and have the ability to synchronize to reference
frequency sources.

This patch series is a prerequisite for EEC object and adds ability
to enable recovered clocks in the physical layer of the netdev object.
Recovered clocks can be used as one of the reference signal by the EEC.

Further work is required to add the DPLL subsystem, link it to the
netdev object and create API to read the EEC DPLL state.

v5:
- rewritten the documentation
- fixed doxygen headers

v4:
- Dropped EEC_STATE reporting (TBD: DPLL subsystem)
- moved recovered clock configuration to ethtool netlink

v3:
- remove RTM_GETRCLKRANGE
- return state of all possible pins in the RTM_GETRCLKSTATE
- clarify documentation

v2:
- improved documentation
- fixed kdoc warning

RFC history:
v2:
- removed whitespace changes
- fix issues reported by test robot
v3:
- Changed naming from SyncE to EEC
- Clarify cover letter and commit message for patch 1
v4:
- Removed sync_source and pin_idx info
- Changed one structure to attributes
- Added EEC_SRC_PORT flag to indicate that the EEC is synchronized
  to the recovered clock of a port that returns the state
v5:
- add EEC source as an optiona attribute
- implement support for recovered clocks
- align states returned by EEC to ITU-T G.781
v6:
- fix EEC clock state reporting
- add documentation
- fix descriptions in code comments


Maciej Machnikowski (4):
  ice: add support detecting features based on netlist
  ethtool: Add ability to configure recovered clock for SyncE feature
  ice: add support for monitoring SyncE DPLL state
  ice: add support for recovered clocks

 Documentation/networking/ethtool-netlink.rst  |  62 ++++
 drivers/net/ethernet/intel/ice/ice.h          |   7 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  70 ++++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 224 +++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  20 +-
 drivers/net/ethernet/intel/ice/ice_devids.h   |   3 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |  96 +++++++
 drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  35 +++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  49 ++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  36 +++
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 include/linux/ethtool.h                       |   9 +
 include/uapi/linux/ethtool_netlink.h          |  21 ++
 net/ethtool/Makefile                          |   3 +-
 net/ethtool/netlink.c                         |  20 ++
 net/ethtool/netlink.h                         |   4 +
 net/ethtool/synce.c                           | 267 ++++++++++++++++++
 18 files changed, 929 insertions(+), 4 deletions(-)
 create mode 100644 net/ethtool/synce.c

-- 
2.26.3


^ permalink raw reply	[flat|nested] 30+ messages in thread
* [PATCH v5 net-next 1/4] ice: add support detecting features based on netlist
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-10 13:45   ` Maciej Machnikowski
  -1 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: maciej.machnikowski, netdev, intel-wired-lan, arkadiusz.kubalewski
  Cc: richardcochran, abyagowi, anthony.l.nguyen, davem, kuba,
	linux-kselftest, idosch, mkubecek, saeed, michael.chan, petrm

Add new functions to check netlist of a given board for:
- Recovered Clock device,
- Clock Generation Unit,
- Clock Multiplexer,

Initialize feature bits depending on detected components.

Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h          |   2 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   7 +-
 drivers/net/ethernet/intel/ice/ice_common.c   | 123 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |   9 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |   1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 7 files changed, 147 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index b67ad51cbcc9..cb6b4c53584b 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -183,6 +183,8 @@
 
 enum ice_feature {
 	ICE_F_DSCP,
+	ICE_F_CGU,
+	ICE_F_PHY_RCLK,
 	ICE_F_SMA_CTRL,
 	ICE_F_MAX
 };
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 4eef3488d86f..339c2a86f680 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1297,6 +1297,8 @@ struct ice_aqc_link_topo_params {
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_CAGE	6
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_MEZZ	7
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_ID_EEPROM	8
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL	9
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX	10
 #define ICE_AQC_LINK_TOPO_NODE_CTX_S		4
 #define ICE_AQC_LINK_TOPO_NODE_CTX_M		\
 				(0xF << ICE_AQC_LINK_TOPO_NODE_CTX_S)
@@ -1333,7 +1335,10 @@ struct ice_aqc_link_topo_addr {
 struct ice_aqc_get_link_topo {
 	struct ice_aqc_link_topo_addr addr;
 	u8 node_part_num;
-#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575	0x21
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575		0x21
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032	0x24
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_PKVL		0x31
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX	0x47
 	u8 rsvd[9];
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index b3066d0fea8b..35903b282885 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -274,6 +274,79 @@ ice_aq_get_link_topo_handle(struct ice_port_info *pi, u8 node_type,
 	return ice_aq_send_cmd(pi->hw, &desc, NULL, 0, cd);
 }
 
+/**
+ * ice_aq_get_netlist_node
+ * @hw: pointer to the hw struct
+ * @cmd: get_link_topo AQ structure
+ * @node_part_number: output node part number if node found
+ * @node_handle: output node handle parameter if node found
+ */
+enum ice_status
+ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
+			u8 *node_part_number, u16 *node_handle)
+{
+	struct ice_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_link_topo);
+	desc.params.get_link_topo = *cmd;
+
+	if (ice_aq_send_cmd(hw, &desc, NULL, 0, NULL))
+		return ICE_ERR_NOT_SUPPORTED;
+
+	if (node_handle)
+		*node_handle =
+			le16_to_cpu(desc.params.get_link_topo.addr.handle);
+	if (node_part_number)
+		*node_part_number = desc.params.get_link_topo.node_part_num;
+
+	return ICE_SUCCESS;
+}
+
+#define MAX_NETLIST_SIZE 10
+/**
+ * ice_find_netlist_node
+ * @hw: pointer to the hw struct
+ * @node_type_ctx: type of netlist node to look for
+ * @node_part_number: node part number to look for
+ * @node_handle: output parameter if node found - optional
+ *
+ * Find and return the node handle for a given node type and part number in the
+ * netlist. When found ICE_SUCCESS is returned, ICE_ERR_DOES_NOT_EXIST
+ * otherwise. If @node_handle provided, it would be set to found node handle.
+ */
+enum ice_status
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle)
+{
+	struct ice_aqc_get_link_topo cmd;
+	u8 rec_node_part_number;
+	enum ice_status status;
+	u16 rec_node_handle;
+	u8 idx;
+
+	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
+		memset(&cmd, 0, sizeof(cmd));
+
+		cmd.addr.topo_params.node_type_ctx =
+			(node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S);
+		cmd.addr.topo_params.index = idx;
+
+		status = ice_aq_get_netlist_node(hw, &cmd,
+						 &rec_node_part_number,
+						 &rec_node_handle);
+		if (status)
+			return status;
+
+		if (rec_node_part_number == node_part_number) {
+			if (node_handle)
+				*node_handle = rec_node_handle;
+			return ICE_SUCCESS;
+		}
+	}
+
+	return ICE_ERR_DOES_NOT_EXIST;
+}
+
 /**
  * ice_is_media_cage_present
  * @pi: port information structure
@@ -5083,3 +5156,53 @@ bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw)
 	}
 	return false;
 }
+
+/**
+ * ice_is_phy_rclk_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the PHY Recovered Clock device is present in the netlist
+ */
+bool ice_is_phy_rclk_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_PKVL, NULL))
+		return false;
+
+	return true;
+}
+
+/**
+ * ice_is_cgu_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the Clock Generation Unit (CGU) device is present in the netlist
+ */
+bool ice_is_cgu_present_e810t(struct ice_hw *hw)
+{
+	if (!ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+				   ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032,
+				   NULL)) {
+		hw->cgu_part_number =
+			ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032;
+		return true;
+	}
+	return false;
+}
+
+/**
+ * ice_is_clock_mux_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the Clock Multiplexer device is present in the netlist
+ */
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX,
+				  NULL))
+		return false;
+
+	return true;
+}
+
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 65c1b3244264..b20a5c085246 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -89,6 +89,12 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
 		    struct ice_aqc_get_phy_caps_data *caps,
 		    struct ice_sq_cd *cd);
 enum ice_status
+ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
+			u8 *node_part_number, u16 *node_handle);
+enum ice_status
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle);
+enum ice_status
 ice_aq_list_caps(struct ice_hw *hw, void *buf, u16 buf_size, u32 *cap_count,
 		 enum ice_adminq_opc opc, struct ice_sq_cd *cd);
 enum ice_status
@@ -206,4 +212,7 @@ bool ice_fw_supports_lldp_fltr_ctrl(struct ice_hw *hw);
 enum ice_status
 ice_lldp_fltr_add_remove(struct ice_hw *hw, u16 vsi_num, bool add);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
+bool ice_is_phy_rclk_present_e810t(struct ice_hw *hw);
+bool ice_is_cgu_present_e810t(struct ice_hw *hw);
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 09a3297cd63c..18c30b2912e3 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -4188,8 +4188,12 @@ void ice_init_feature_support(struct ice_pf *pf)
 	case ICE_DEV_ID_E810C_QSFP:
 	case ICE_DEV_ID_E810C_SFP:
 		ice_set_feature_support(pf, ICE_F_DSCP);
-		if (ice_is_e810t(&pf->hw))
+		if (ice_is_clock_mux_present_e810t(&pf->hw))
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
+		if (ice_is_phy_rclk_present_e810t(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_PHY_RCLK);
+		if (ice_is_cgu_present_e810t(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_CGU);
 		break;
 	default:
 		break;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 29f947c0cd2e..aa257db36765 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -800,3 +800,4 @@ bool ice_is_pca9575_present(struct ice_hw *hw)
 
 	return !status && handle;
 }
+
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 9e0c2923c62e..a9dc16641bd4 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -920,6 +920,7 @@ struct ice_hw {
 	struct list_head rss_list_head;
 	struct ice_mbx_snapshot mbx_snapshot;
 	u16 io_expander_handle;
+	u8 cgu_part_number;
 };
 
 /* Statistics collected by each port, VSI, VEB, and S-channel */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 1/4] ice: add support detecting features based on netlist
@ 2021-12-10 13:45   ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: intel-wired-lan

Add new functions to check netlist of a given board for:
- Recovered Clock device,
- Clock Generation Unit,
- Clock Multiplexer,

Initialize feature bits depending on detected components.

Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h          |   2 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   7 +-
 drivers/net/ethernet/intel/ice/ice_common.c   | 123 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |   9 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |   1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 7 files changed, 147 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index b67ad51cbcc9..cb6b4c53584b 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -183,6 +183,8 @@
 
 enum ice_feature {
 	ICE_F_DSCP,
+	ICE_F_CGU,
+	ICE_F_PHY_RCLK,
 	ICE_F_SMA_CTRL,
 	ICE_F_MAX
 };
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 4eef3488d86f..339c2a86f680 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1297,6 +1297,8 @@ struct ice_aqc_link_topo_params {
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_CAGE	6
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_MEZZ	7
 #define ICE_AQC_LINK_TOPO_NODE_TYPE_ID_EEPROM	8
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL	9
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX	10
 #define ICE_AQC_LINK_TOPO_NODE_CTX_S		4
 #define ICE_AQC_LINK_TOPO_NODE_CTX_M		\
 				(0xF << ICE_AQC_LINK_TOPO_NODE_CTX_S)
@@ -1333,7 +1335,10 @@ struct ice_aqc_link_topo_addr {
 struct ice_aqc_get_link_topo {
 	struct ice_aqc_link_topo_addr addr;
 	u8 node_part_num;
-#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575	0x21
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575		0x21
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032	0x24
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_PKVL		0x31
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX	0x47
 	u8 rsvd[9];
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index b3066d0fea8b..35903b282885 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -274,6 +274,79 @@ ice_aq_get_link_topo_handle(struct ice_port_info *pi, u8 node_type,
 	return ice_aq_send_cmd(pi->hw, &desc, NULL, 0, cd);
 }
 
+/**
+ * ice_aq_get_netlist_node
+ * @hw: pointer to the hw struct
+ * @cmd: get_link_topo AQ structure
+ * @node_part_number: output node part number if node found
+ * @node_handle: output node handle parameter if node found
+ */
+enum ice_status
+ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
+			u8 *node_part_number, u16 *node_handle)
+{
+	struct ice_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_link_topo);
+	desc.params.get_link_topo = *cmd;
+
+	if (ice_aq_send_cmd(hw, &desc, NULL, 0, NULL))
+		return ICE_ERR_NOT_SUPPORTED;
+
+	if (node_handle)
+		*node_handle =
+			le16_to_cpu(desc.params.get_link_topo.addr.handle);
+	if (node_part_number)
+		*node_part_number = desc.params.get_link_topo.node_part_num;
+
+	return ICE_SUCCESS;
+}
+
+#define MAX_NETLIST_SIZE 10
+/**
+ * ice_find_netlist_node
+ * @hw: pointer to the hw struct
+ * @node_type_ctx: type of netlist node to look for
+ * @node_part_number: node part number to look for
+ * @node_handle: output parameter if node found - optional
+ *
+ * Find and return the node handle for a given node type and part number in the
+ * netlist. When found ICE_SUCCESS is returned, ICE_ERR_DOES_NOT_EXIST
+ * otherwise. If @node_handle provided, it would be set to found node handle.
+ */
+enum ice_status
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle)
+{
+	struct ice_aqc_get_link_topo cmd;
+	u8 rec_node_part_number;
+	enum ice_status status;
+	u16 rec_node_handle;
+	u8 idx;
+
+	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
+		memset(&cmd, 0, sizeof(cmd));
+
+		cmd.addr.topo_params.node_type_ctx =
+			(node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S);
+		cmd.addr.topo_params.index = idx;
+
+		status = ice_aq_get_netlist_node(hw, &cmd,
+						 &rec_node_part_number,
+						 &rec_node_handle);
+		if (status)
+			return status;
+
+		if (rec_node_part_number == node_part_number) {
+			if (node_handle)
+				*node_handle = rec_node_handle;
+			return ICE_SUCCESS;
+		}
+	}
+
+	return ICE_ERR_DOES_NOT_EXIST;
+}
+
 /**
  * ice_is_media_cage_present
  * @pi: port information structure
@@ -5083,3 +5156,53 @@ bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw)
 	}
 	return false;
 }
+
+/**
+ * ice_is_phy_rclk_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the PHY Recovered Clock device is present in the netlist
+ */
+bool ice_is_phy_rclk_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_PKVL, NULL))
+		return false;
+
+	return true;
+}
+
+/**
+ * ice_is_cgu_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the Clock Generation Unit (CGU) device is present in the netlist
+ */
+bool ice_is_cgu_present_e810t(struct ice_hw *hw)
+{
+	if (!ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+				   ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032,
+				   NULL)) {
+		hw->cgu_part_number =
+			ICE_ACQ_GET_LINK_TOPO_NODE_NR_ZL30632_80032;
+		return true;
+	}
+	return false;
+}
+
+/**
+ * ice_is_clock_mux_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the Clock Multiplexer device is present in the netlist
+ */
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_MUX,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX,
+				  NULL))
+		return false;
+
+	return true;
+}
+
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 65c1b3244264..b20a5c085246 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -89,6 +89,12 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
 		    struct ice_aqc_get_phy_caps_data *caps,
 		    struct ice_sq_cd *cd);
 enum ice_status
+ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
+			u8 *node_part_number, u16 *node_handle);
+enum ice_status
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle);
+enum ice_status
 ice_aq_list_caps(struct ice_hw *hw, void *buf, u16 buf_size, u32 *cap_count,
 		 enum ice_adminq_opc opc, struct ice_sq_cd *cd);
 enum ice_status
@@ -206,4 +212,7 @@ bool ice_fw_supports_lldp_fltr_ctrl(struct ice_hw *hw);
 enum ice_status
 ice_lldp_fltr_add_remove(struct ice_hw *hw, u16 vsi_num, bool add);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
+bool ice_is_phy_rclk_present_e810t(struct ice_hw *hw);
+bool ice_is_cgu_present_e810t(struct ice_hw *hw);
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 09a3297cd63c..18c30b2912e3 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -4188,8 +4188,12 @@ void ice_init_feature_support(struct ice_pf *pf)
 	case ICE_DEV_ID_E810C_QSFP:
 	case ICE_DEV_ID_E810C_SFP:
 		ice_set_feature_support(pf, ICE_F_DSCP);
-		if (ice_is_e810t(&pf->hw))
+		if (ice_is_clock_mux_present_e810t(&pf->hw))
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
+		if (ice_is_phy_rclk_present_e810t(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_PHY_RCLK);
+		if (ice_is_cgu_present_e810t(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_CGU);
 		break;
 	default:
 		break;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 29f947c0cd2e..aa257db36765 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -800,3 +800,4 @@ bool ice_is_pca9575_present(struct ice_hw *hw)
 
 	return !status && handle;
 }
+
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 9e0c2923c62e..a9dc16641bd4 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -920,6 +920,7 @@ struct ice_hw {
 	struct list_head rss_list_head;
 	struct ice_mbx_snapshot mbx_snapshot;
 	u16 io_expander_handle;
+	u8 cgu_part_number;
 };
 
 /* Statistics collected by each port, VSI, VEB, and S-channel */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v5 net-next 2/4] ethtool: Add ability to configure recovered clock for SyncE feature
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-10 13:45   ` Maciej Machnikowski
  -1 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: maciej.machnikowski, netdev, intel-wired-lan, arkadiusz.kubalewski
  Cc: richardcochran, abyagowi, anthony.l.nguyen, davem, kuba,
	linux-kselftest, idosch, mkubecek, saeed, michael.chan, petrm

Add netlink ethtool messages:
- ETHTOOL_MSG_RCLK_GET
- ETHTOOL_MSG_RCLK_SET
Required for controling basic SyncE functionality - configuration of
recovered reference clocks from PHY port on netdevice supporting SyncE.

- ETHTOOL_MSG_RCLK_GET - will read the current state if pin index is
			given or will return allowed range if it's not

- ETHTOOL_MSG_RCLK_SET - will configure given recovered clock pin to
			output recovered clock

Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 Documentation/networking/ethtool-netlink.rst |  62 +++++
 include/linux/ethtool.h                      |   9 +
 include/uapi/linux/ethtool_netlink.h         |  21 ++
 net/ethtool/Makefile                         |   3 +-
 net/ethtool/netlink.c                        |  20 ++
 net/ethtool/netlink.h                        |   4 +
 net/ethtool/synce.c                          | 267 +++++++++++++++++++
 7 files changed, 385 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/synce.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 9d98e0511249..6399cdb3fdca 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -220,6 +220,8 @@ Userspace to kernel:
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET``       get PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_SET``            set transceiver module parameters
   ``ETHTOOL_MSG_MODULE_GET``            get transceiver module parameters
+  ``ETHTOOL_RCLK_GET``                  get recovered clock parameters
+  ``ETHTOOL_RCLK_SET``                  set recovered clock parameters
   ===================================== =================================
 
 Kernel to userspace:
@@ -260,6 +262,8 @@ Kernel to userspace:
   ``ETHTOOL_MSG_STATS_GET_REPLY``          standard statistics
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY``    PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
+  ``ETHTOOL_MSG_RCLK_GET_REPLY``           reference recovered clock config
+  ``ETHTOOL_MSG_RCLK_NTF``                 reference recovered clock config
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -1598,6 +1602,62 @@ For SFF-8636 modules, low power mode is forced by the host according to table
 For CMIS modules, low power mode is forced by the host according to table 6-12
 in revision 5.0 of the specification.
 
+RCLK_GET
+========
+
+Get status of an output pin for PHY recovered frequency clock signal.
+
+Request contents:
+
+  ======================================  ======  ==========================
+  ``ETHTOOL_A_RCLK_HEADER``               nested  request header
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ======================================  ======  ==========================
+
+Kernel response contents:
+
+  ======================================  ======  ==========================
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ``ETHTOOL_A_RCLK_PIN_FLAGS``            u32     state of a pin
+  ``ETHTOOL_A_RCLK_RANGE_MIN_PIN``        u32     min index of RCLK pins
+  ``ETHTOOL_A_RCLK_RANGE_MAX_PIN``        u32     max index of RCLK pins
+  ======================================  ======  ==========================
+
+A device can support multiple recovered clock output pins. They typically are
+connected to the EEC and are used as a frequency reference.
+When a given pin on a given port is enabled, the PHY recovered frequency is
+fed onto that pin.
+Pins can be freely indexed and don't have to start with index 0.
+
+The ``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is optional parameter. If present in
+the RCLK_GET request, the ``ETHTOOL_A_RCLK_PIN_FLAGS`` will be returned in a
+response, containing the state of the pin pointed by the index.
+
+If ``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is not present in the RCLK_GET request,
+the range of available pins is returned:
+``ETHTOOL_A_RCLK_RANGE_MIN_PIN`` is the lowest recovered clock pin index.
+``ETHTOOL_A_RCLK_RANGE_MAX_PIN`` is the highest recovered clock pin index.
+
+RCLK_SET
+==========
+
+Set status of an output pin for PHY recovered frequency clock.
+
+Request contents:
+
+  ======================================  ======  ========================
+  ``ETHTOOL_A_RCLK_HEADER``               nested  request header
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ``ETHTOOL_A_RCLK_PIN_FLAGS``            u32     requested state
+  ======================================  ======  ========================
+
+``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is the index of the pin for which a change of
+state is requested.
+``ETHTOOL_A_RCLK_PIN_FLAGS`` if the ``ETHTOOL_RCLK_PIN_FLAGS_ENA`` flag
+is set, the given output pin will be enabled, if not - the pin will be disabled
+
+
+
 Request translation
 ===================
 
@@ -1699,4 +1759,6 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PHC_VCLOCKS_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_SET``
+  n/a                                 ``ETHTOOL_MSG_RCLK_GET``
+  n/a                                 ``ETHTOOL_MSG_RCLK_SET``
   =================================== =====================================
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a26f37a27167..e344e5153f9b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -614,6 +614,9 @@ struct ethtool_module_power_mode_params {
  *	plugged-in.
  * @set_module_power_mode: Set the power mode policy for the plug-in module
  *	used by the network device.
+ * @get_rclk_range: Get range of valid Reference Clock input pins for the port.
+ * @get_rclk_state: Get state of Reference Clock input signal pin.
+ * @set_rclk_out: Enable/disable Reference Clock input signal pin.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -750,6 +753,12 @@ struct ethtool_ops {
 	int	(*set_module_power_mode)(struct net_device *dev,
 					 const struct ethtool_module_power_mode_params *params,
 					 struct netlink_ext_ack *extack);
+	int	(*get_rclk_range)(struct net_device *dev, u32 *min_idx,
+				  u32 *max_idx, struct netlink_ext_ack *extack);
+	int	(*get_rclk_state)(struct net_device *dev, u32 out_idx,
+				  bool *ena, struct netlink_ext_ack *extack);
+	int	(*set_rclk_out)(struct net_device *dev, u32 out_idx, bool ena,
+				struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index cca6e474a085..9a860f052a36 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -49,6 +49,8 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET,
 	ETHTOOL_MSG_MODULE_GET,
 	ETHTOOL_MSG_MODULE_SET,
+	ETHTOOL_MSG_RCLK_GET,
+	ETHTOOL_MSG_RCLK_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -94,6 +96,8 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY,
 	ETHTOOL_MSG_MODULE_GET_REPLY,
 	ETHTOOL_MSG_MODULE_NTF,
+	ETHTOOL_MSG_RCLK_GET_REPLY,
+	ETHTOOL_MSG_RCLK_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -853,6 +857,23 @@ enum {
 	ETHTOOL_A_MODULE_MAX = (__ETHTOOL_A_MODULE_CNT - 1)
 };
 
+/* REF CLK */
+
+enum {
+	ETHTOOL_A_RCLK_UNSPEC,
+	ETHTOOL_A_RCLK_HEADER,		/* nest - _A_HEADER_* */
+	ETHTOOL_A_RCLK_OUT_PIN_IDX,	/* u32 */
+	ETHTOOL_A_RCLK_PIN_FLAGS,	/* u32 */
+	ETHTOOL_A_RCLK_PIN_MIN,		/* u32 */
+	ETHTOOL_A_RCLK_PIN_MAX,		/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_RCLK_CNT,
+	ETHTOOL_A_RCLK_MAX = (__ETHTOOL_A_RCLK_CNT - 1)
+};
+
+#define ETHTOOL_RCLK_PIN_FLAGS_ENA	(1 << 0)
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index b76432e70e6b..dd6de311a9c2 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
-		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o
+		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o \
+		   synce.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 23f32a995099..a88e4e91d015 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -285,6 +285,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_STATS_GET]		= &ethnl_stats_request_ops,
 	[ETHTOOL_MSG_PHC_VCLOCKS_GET]	= &ethnl_phc_vclocks_request_ops,
 	[ETHTOOL_MSG_MODULE_GET]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_RCLK_GET]		= &ethnl_rclk_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -597,6 +598,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_FEC_NTF]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_RCLK_NTF]		= &ethnl_rclk_request_ops,
 };
 
 /* default notification handler */
@@ -692,6 +694,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_FEC_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
+	[ETHTOOL_MSG_RCLK_NTF]		= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1021,6 +1024,23 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_module_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_module_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_RCLK_GET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_rclk_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_rclk_get_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_RCLK_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_rclk,
+		.policy = ethnl_rclk_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_rclk_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index a779bbb0c524..cfc7c45513b1 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -340,6 +340,7 @@ extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops;
 extern const struct ethnl_request_ops ethnl_stats_request_ops;
 extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops;
 extern const struct ethnl_request_ops ethnl_module_request_ops;
+extern const struct ethnl_request_ops ethnl_rclk_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -378,6 +379,8 @@ extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1
 extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1];
 extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1];
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
+extern const struct nla_policy ethnl_rclk_get_policy[ETHTOOL_A_RCLK_OUT_PIN_IDX + 1];
+extern const struct nla_policy ethnl_rclk_set_policy[ETHTOOL_A_RCLK_PIN_FLAGS + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -397,6 +400,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_module(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_rclk(struct sk_buff *skb, struct genl_info *info);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/synce.c b/net/ethtool/synce.c
new file mode 100644
index 000000000000..f4ebb4c57d4d
--- /dev/null
+++ b/net/ethtool/synce.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include "netlink.h"
+
+struct rclk_out_pin_info {
+	u32 idx;
+	bool valid;
+};
+
+struct rclk_request_data {
+	struct ethnl_req_info base;
+	struct rclk_out_pin_info out_pin;
+};
+
+struct rclk_pin_state_info {
+	u32 range_min;
+	u32 range_max;
+	u32 flags;
+	u32 idx;
+};
+
+struct rclk_reply_data {
+	struct ethnl_reply_data	base;
+	struct rclk_pin_state_info pin_state;
+};
+
+#define RCLK_REPDATA(__reply_base) \
+	container_of(__reply_base, struct rclk_reply_data, base)
+
+#define RCLK_REQDATA(__req_base) \
+	container_of(__req_base, struct rclk_request_data, base)
+
+/* RCLK_GET */
+
+const struct nla_policy
+ethnl_rclk_get_policy[ETHTOOL_A_RCLK_OUT_PIN_IDX + 1] = {
+	[ETHTOOL_A_RCLK_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_RCLK_OUT_PIN_IDX] = { .type = NLA_U32 },
+};
+
+static int rclk_parse_request(struct ethnl_req_info *req_base,
+			      struct nlattr **tb,
+			      struct netlink_ext_ack *extack)
+{
+	struct rclk_request_data *req = RCLK_REQDATA(req_base);
+
+	if (tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]) {
+		req->out_pin.idx = nla_get_u32(tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]);
+		req->out_pin.valid = true;
+	}
+
+	return 0;
+}
+
+static int rclk_state_get(struct net_device *dev,
+			  struct rclk_reply_data *data,
+			  struct netlink_ext_ack *extack,
+			  u32 out_idx)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	bool pin_state;
+	int ret;
+
+	if (!ops->get_rclk_state)
+		return -EOPNOTSUPP;
+
+	ret = ops->get_rclk_state(dev, out_idx, &pin_state, extack);
+	if (ret)
+		return ret;
+
+	data->pin_state.flags = pin_state ? ETHTOOL_RCLK_PIN_FLAGS_ENA : 0;
+	data->pin_state.idx = out_idx;
+
+	return ret;
+}
+
+static int rclk_range_get(struct net_device *dev,
+			  struct rclk_reply_data *data,
+			  struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u32 min_idx, max_idx;
+	int ret;
+
+	if (!ops->get_rclk_range)
+		return -EOPNOTSUPP;
+
+	ret = ops->get_rclk_range(dev, &min_idx, &max_idx, extack);
+	if (ret)
+		return ret;
+
+	data->pin_state.range_min = min_idx;
+	data->pin_state.range_max = max_idx;
+
+	return ret;
+}
+
+static int rclk_prepare_data(const struct ethnl_req_info *req_base,
+			     struct ethnl_reply_data *reply_base,
+			     struct genl_info *info)
+{
+	struct rclk_reply_data *reply = RCLK_REPDATA(reply_base);
+	struct rclk_request_data *request = RCLK_REQDATA(req_base);
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	memset(&reply->pin_state, 0, sizeof(reply->pin_state));
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	if (request->out_pin.valid)
+		ret = rclk_state_get(dev, reply, extack,
+				     request->out_pin.idx);
+	else
+		ret = rclk_range_get(dev, reply, extack);
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int rclk_fill_reply(struct sk_buff *skb,
+			   const struct ethnl_req_info *req_base,
+			   const struct ethnl_reply_data *reply_base)
+{
+	const struct rclk_reply_data *reply = RCLK_REPDATA(reply_base);
+	const struct rclk_request_data *request = RCLK_REQDATA(req_base);
+
+	if (request->out_pin.valid) {
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_FLAGS,
+				reply->pin_state.flags))
+			return -EMSGSIZE;
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_OUT_PIN_IDX,
+				reply->pin_state.idx))
+			return -EMSGSIZE;
+	} else {
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_MIN,
+				reply->pin_state.range_min))
+			return -EMSGSIZE;
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_MAX,
+				reply->pin_state.range_max))
+			return -EMSGSIZE;
+	}
+
+	return 0;
+}
+
+static int rclk_reply_size(const struct ethnl_req_info *req_base,
+			   const struct ethnl_reply_data *reply_base)
+{
+	const struct rclk_request_data *request = RCLK_REQDATA(req_base);
+
+	if (request->out_pin.valid)
+		return nla_total_size(sizeof(u32)) +	/* ETHTOOL_A_RCLK_PIN_FLAGS */
+		       nla_total_size(sizeof(u32));	/* ETHTOOL_A_RCLK_OUT_PIN_IDX */
+	else
+		return nla_total_size(sizeof(u32)) +	/* ETHTOOL_A_RCLK_PIN_MIN */
+		       nla_total_size(sizeof(u32));	/* ETHTOOL_A_RCLK_PIN_MAX */
+}
+
+const struct ethnl_request_ops ethnl_rclk_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_RCLK_GET,
+	.reply_cmd		= ETHTOOL_MSG_RCLK_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_RCLK_HEADER,
+	.req_info_size		= sizeof(struct rclk_request_data),
+	.reply_data_size	= sizeof(struct rclk_reply_data),
+
+	.parse_request		= rclk_parse_request,
+	.prepare_data		= rclk_prepare_data,
+	.reply_size		= rclk_reply_size,
+	.fill_reply		= rclk_fill_reply,
+};
+
+/* RCLK SET */
+
+const struct nla_policy
+ethnl_rclk_set_policy[ETHTOOL_A_RCLK_PIN_FLAGS + 1] = {
+	[ETHTOOL_A_RCLK_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_RCLK_OUT_PIN_IDX] = { .type = NLA_U32 },
+	[ETHTOOL_A_RCLK_PIN_FLAGS] = { .type = NLA_U32 },
+};
+
+static int rclk_set_state(struct net_device *dev, struct nlattr **tb,
+			  bool *p_mod, struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	bool old_state, new_state;
+	u32 min_idx, max_idx;
+	u32 out_idx;
+	int ret;
+
+	if (!tb[ETHTOOL_A_RCLK_PIN_FLAGS] &&
+	    !tb[ETHTOOL_A_RCLK_OUT_PIN_IDX])
+		return 0;
+
+	if (!ops->set_rclk_out || !ops->get_rclk_range) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[ETHTOOL_A_RCLK_PIN_FLAGS],
+				    "Setting recovered clock state is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	ret = ops->get_rclk_range(dev, &min_idx, &max_idx, extack);
+	if (ret)
+		return ret;
+
+	out_idx = nla_get_u32(tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]);
+	if (out_idx < min_idx || out_idx > max_idx) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[ETHTOOL_A_RCLK_OUT_PIN_IDX],
+				    "Requested recovered clock pin index is out of range");
+		return -EINVAL;
+	}
+
+	ret = ops->get_rclk_state(dev, out_idx, &old_state, extack);
+	if (ret < 0)
+		return ret;
+
+	new_state = !!(nla_get_u32(tb[ETHTOOL_A_RCLK_PIN_FLAGS]) &
+		       ETHTOOL_RCLK_PIN_FLAGS_ENA);
+
+	/* If state changed - flag need for sending the notification */
+	*p_mod = old_state != new_state;
+
+	return ops->set_rclk_out(dev, out_idx, new_state, extack);
+}
+
+int ethnl_set_rclk(struct sk_buff *skb, struct genl_info *info)
+{
+	struct ethnl_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct net_device *dev;
+	bool mod = false;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info, tb[ETHTOOL_A_RCLK_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+	dev = req_info.dev;
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = rclk_set_state(dev, tb, &mod, info->extack);
+	if (ret < 0)
+		goto out_ops;
+
+	if (!mod)
+		goto out_ops;
+
+	ethtool_notify(dev, ETHTOOL_MSG_RCLK_NTF, NULL);
+
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+	dev_put(dev);
+	return ret;
+}
+
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 2/4] ethtool: Add ability to configure recovered clock for SyncE feature
@ 2021-12-10 13:45   ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: intel-wired-lan

Add netlink ethtool messages:
- ETHTOOL_MSG_RCLK_GET
- ETHTOOL_MSG_RCLK_SET
Required for controling basic SyncE functionality - configuration of
recovered reference clocks from PHY port on netdevice supporting SyncE.

- ETHTOOL_MSG_RCLK_GET - will read the current state if pin index is
			given or will return allowed range if it's not

- ETHTOOL_MSG_RCLK_SET - will configure given recovered clock pin to
			output recovered clock

Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 Documentation/networking/ethtool-netlink.rst |  62 +++++
 include/linux/ethtool.h                      |   9 +
 include/uapi/linux/ethtool_netlink.h         |  21 ++
 net/ethtool/Makefile                         |   3 +-
 net/ethtool/netlink.c                        |  20 ++
 net/ethtool/netlink.h                        |   4 +
 net/ethtool/synce.c                          | 267 +++++++++++++++++++
 7 files changed, 385 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/synce.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 9d98e0511249..6399cdb3fdca 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -220,6 +220,8 @@ Userspace to kernel:
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET``       get PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_SET``            set transceiver module parameters
   ``ETHTOOL_MSG_MODULE_GET``            get transceiver module parameters
+  ``ETHTOOL_RCLK_GET``                  get recovered clock parameters
+  ``ETHTOOL_RCLK_SET``                  set recovered clock parameters
   ===================================== =================================
 
 Kernel to userspace:
@@ -260,6 +262,8 @@ Kernel to userspace:
   ``ETHTOOL_MSG_STATS_GET_REPLY``          standard statistics
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY``    PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
+  ``ETHTOOL_MSG_RCLK_GET_REPLY``           reference recovered clock config
+  ``ETHTOOL_MSG_RCLK_NTF``                 reference recovered clock config
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -1598,6 +1602,62 @@ For SFF-8636 modules, low power mode is forced by the host according to table
 For CMIS modules, low power mode is forced by the host according to table 6-12
 in revision 5.0 of the specification.
 
+RCLK_GET
+========
+
+Get status of an output pin for PHY recovered frequency clock signal.
+
+Request contents:
+
+  ======================================  ======  ==========================
+  ``ETHTOOL_A_RCLK_HEADER``               nested  request header
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ======================================  ======  ==========================
+
+Kernel response contents:
+
+  ======================================  ======  ==========================
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ``ETHTOOL_A_RCLK_PIN_FLAGS``            u32     state of a pin
+  ``ETHTOOL_A_RCLK_RANGE_MIN_PIN``        u32     min index of RCLK pins
+  ``ETHTOOL_A_RCLK_RANGE_MAX_PIN``        u32     max index of RCLK pins
+  ======================================  ======  ==========================
+
+A device can support multiple recovered clock output pins. They typically are
+connected to the EEC and are used as a frequency reference.
+When a given pin on a given port is enabled, the PHY recovered frequency is
+fed onto that pin.
+Pins can be freely indexed and don't have to start with index 0.
+
+The ``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is optional parameter. If present in
+the RCLK_GET request, the ``ETHTOOL_A_RCLK_PIN_FLAGS`` will be returned in a
+response, containing the state of the pin pointed by the index.
+
+If ``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is not present in the RCLK_GET request,
+the range of available pins is returned:
+``ETHTOOL_A_RCLK_RANGE_MIN_PIN`` is the lowest recovered clock pin index.
+``ETHTOOL_A_RCLK_RANGE_MAX_PIN`` is the highest recovered clock pin index.
+
+RCLK_SET
+==========
+
+Set status of an output pin for PHY recovered frequency clock.
+
+Request contents:
+
+  ======================================  ======  ========================
+  ``ETHTOOL_A_RCLK_HEADER``               nested  request header
+  ``ETHTOOL_A_RCLK_OUT_PIN_IDX``          u32     index of a pin
+  ``ETHTOOL_A_RCLK_PIN_FLAGS``            u32     requested state
+  ======================================  ======  ========================
+
+``ETHTOOL_A_RCLK_OUT_PIN_IDX`` is the index of the pin for which a change of
+state is requested.
+``ETHTOOL_A_RCLK_PIN_FLAGS`` if the ``ETHTOOL_RCLK_PIN_FLAGS_ENA`` flag
+is set, the given output pin will be enabled, if not - the pin will be disabled
+
+
+
 Request translation
 ===================
 
@@ -1699,4 +1759,6 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PHC_VCLOCKS_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_SET``
+  n/a                                 ``ETHTOOL_MSG_RCLK_GET``
+  n/a                                 ``ETHTOOL_MSG_RCLK_SET``
   =================================== =====================================
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a26f37a27167..e344e5153f9b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -614,6 +614,9 @@ struct ethtool_module_power_mode_params {
  *	plugged-in.
  * @set_module_power_mode: Set the power mode policy for the plug-in module
  *	used by the network device.
+ * @get_rclk_range: Get range of valid Reference Clock input pins for the port.
+ * @get_rclk_state: Get state of Reference Clock input signal pin.
+ * @set_rclk_out: Enable/disable Reference Clock input signal pin.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -750,6 +753,12 @@ struct ethtool_ops {
 	int	(*set_module_power_mode)(struct net_device *dev,
 					 const struct ethtool_module_power_mode_params *params,
 					 struct netlink_ext_ack *extack);
+	int	(*get_rclk_range)(struct net_device *dev, u32 *min_idx,
+				  u32 *max_idx, struct netlink_ext_ack *extack);
+	int	(*get_rclk_state)(struct net_device *dev, u32 out_idx,
+				  bool *ena, struct netlink_ext_ack *extack);
+	int	(*set_rclk_out)(struct net_device *dev, u32 out_idx, bool ena,
+				struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index cca6e474a085..9a860f052a36 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -49,6 +49,8 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET,
 	ETHTOOL_MSG_MODULE_GET,
 	ETHTOOL_MSG_MODULE_SET,
+	ETHTOOL_MSG_RCLK_GET,
+	ETHTOOL_MSG_RCLK_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -94,6 +96,8 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY,
 	ETHTOOL_MSG_MODULE_GET_REPLY,
 	ETHTOOL_MSG_MODULE_NTF,
+	ETHTOOL_MSG_RCLK_GET_REPLY,
+	ETHTOOL_MSG_RCLK_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -853,6 +857,23 @@ enum {
 	ETHTOOL_A_MODULE_MAX = (__ETHTOOL_A_MODULE_CNT - 1)
 };
 
+/* REF CLK */
+
+enum {
+	ETHTOOL_A_RCLK_UNSPEC,
+	ETHTOOL_A_RCLK_HEADER,		/* nest - _A_HEADER_* */
+	ETHTOOL_A_RCLK_OUT_PIN_IDX,	/* u32 */
+	ETHTOOL_A_RCLK_PIN_FLAGS,	/* u32 */
+	ETHTOOL_A_RCLK_PIN_MIN,		/* u32 */
+	ETHTOOL_A_RCLK_PIN_MAX,		/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_RCLK_CNT,
+	ETHTOOL_A_RCLK_MAX = (__ETHTOOL_A_RCLK_CNT - 1)
+};
+
+#define ETHTOOL_RCLK_PIN_FLAGS_ENA	(1 << 0)
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index b76432e70e6b..dd6de311a9c2 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
-		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o
+		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o \
+		   synce.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 23f32a995099..a88e4e91d015 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -285,6 +285,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_STATS_GET]		= &ethnl_stats_request_ops,
 	[ETHTOOL_MSG_PHC_VCLOCKS_GET]	= &ethnl_phc_vclocks_request_ops,
 	[ETHTOOL_MSG_MODULE_GET]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_RCLK_GET]		= &ethnl_rclk_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -597,6 +598,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_FEC_NTF]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_RCLK_NTF]		= &ethnl_rclk_request_ops,
 };
 
 /* default notification handler */
@@ -692,6 +694,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_FEC_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
+	[ETHTOOL_MSG_RCLK_NTF]		= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1021,6 +1024,23 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_module_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_module_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_RCLK_GET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_rclk_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_rclk_get_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_RCLK_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_rclk,
+		.policy = ethnl_rclk_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_rclk_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index a779bbb0c524..cfc7c45513b1 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -340,6 +340,7 @@ extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops;
 extern const struct ethnl_request_ops ethnl_stats_request_ops;
 extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops;
 extern const struct ethnl_request_ops ethnl_module_request_ops;
+extern const struct ethnl_request_ops ethnl_rclk_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -378,6 +379,8 @@ extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1
 extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1];
 extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1];
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
+extern const struct nla_policy ethnl_rclk_get_policy[ETHTOOL_A_RCLK_OUT_PIN_IDX + 1];
+extern const struct nla_policy ethnl_rclk_set_policy[ETHTOOL_A_RCLK_PIN_FLAGS + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -397,6 +400,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_module(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_rclk(struct sk_buff *skb, struct genl_info *info);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/synce.c b/net/ethtool/synce.c
new file mode 100644
index 000000000000..f4ebb4c57d4d
--- /dev/null
+++ b/net/ethtool/synce.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include "netlink.h"
+
+struct rclk_out_pin_info {
+	u32 idx;
+	bool valid;
+};
+
+struct rclk_request_data {
+	struct ethnl_req_info base;
+	struct rclk_out_pin_info out_pin;
+};
+
+struct rclk_pin_state_info {
+	u32 range_min;
+	u32 range_max;
+	u32 flags;
+	u32 idx;
+};
+
+struct rclk_reply_data {
+	struct ethnl_reply_data	base;
+	struct rclk_pin_state_info pin_state;
+};
+
+#define RCLK_REPDATA(__reply_base) \
+	container_of(__reply_base, struct rclk_reply_data, base)
+
+#define RCLK_REQDATA(__req_base) \
+	container_of(__req_base, struct rclk_request_data, base)
+
+/* RCLK_GET */
+
+const struct nla_policy
+ethnl_rclk_get_policy[ETHTOOL_A_RCLK_OUT_PIN_IDX + 1] = {
+	[ETHTOOL_A_RCLK_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_RCLK_OUT_PIN_IDX] = { .type = NLA_U32 },
+};
+
+static int rclk_parse_request(struct ethnl_req_info *req_base,
+			      struct nlattr **tb,
+			      struct netlink_ext_ack *extack)
+{
+	struct rclk_request_data *req = RCLK_REQDATA(req_base);
+
+	if (tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]) {
+		req->out_pin.idx = nla_get_u32(tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]);
+		req->out_pin.valid = true;
+	}
+
+	return 0;
+}
+
+static int rclk_state_get(struct net_device *dev,
+			  struct rclk_reply_data *data,
+			  struct netlink_ext_ack *extack,
+			  u32 out_idx)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	bool pin_state;
+	int ret;
+
+	if (!ops->get_rclk_state)
+		return -EOPNOTSUPP;
+
+	ret = ops->get_rclk_state(dev, out_idx, &pin_state, extack);
+	if (ret)
+		return ret;
+
+	data->pin_state.flags = pin_state ? ETHTOOL_RCLK_PIN_FLAGS_ENA : 0;
+	data->pin_state.idx = out_idx;
+
+	return ret;
+}
+
+static int rclk_range_get(struct net_device *dev,
+			  struct rclk_reply_data *data,
+			  struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u32 min_idx, max_idx;
+	int ret;
+
+	if (!ops->get_rclk_range)
+		return -EOPNOTSUPP;
+
+	ret = ops->get_rclk_range(dev, &min_idx, &max_idx, extack);
+	if (ret)
+		return ret;
+
+	data->pin_state.range_min = min_idx;
+	data->pin_state.range_max = max_idx;
+
+	return ret;
+}
+
+static int rclk_prepare_data(const struct ethnl_req_info *req_base,
+			     struct ethnl_reply_data *reply_base,
+			     struct genl_info *info)
+{
+	struct rclk_reply_data *reply = RCLK_REPDATA(reply_base);
+	struct rclk_request_data *request = RCLK_REQDATA(req_base);
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	memset(&reply->pin_state, 0, sizeof(reply->pin_state));
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	if (request->out_pin.valid)
+		ret = rclk_state_get(dev, reply, extack,
+				     request->out_pin.idx);
+	else
+		ret = rclk_range_get(dev, reply, extack);
+
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int rclk_fill_reply(struct sk_buff *skb,
+			   const struct ethnl_req_info *req_base,
+			   const struct ethnl_reply_data *reply_base)
+{
+	const struct rclk_reply_data *reply = RCLK_REPDATA(reply_base);
+	const struct rclk_request_data *request = RCLK_REQDATA(req_base);
+
+	if (request->out_pin.valid) {
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_FLAGS,
+				reply->pin_state.flags))
+			return -EMSGSIZE;
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_OUT_PIN_IDX,
+				reply->pin_state.idx))
+			return -EMSGSIZE;
+	} else {
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_MIN,
+				reply->pin_state.range_min))
+			return -EMSGSIZE;
+		if (nla_put_u32(skb, ETHTOOL_A_RCLK_PIN_MAX,
+				reply->pin_state.range_max))
+			return -EMSGSIZE;
+	}
+
+	return 0;
+}
+
+static int rclk_reply_size(const struct ethnl_req_info *req_base,
+			   const struct ethnl_reply_data *reply_base)
+{
+	const struct rclk_request_data *request = RCLK_REQDATA(req_base);
+
+	if (request->out_pin.valid)
+		return nla_total_size(sizeof(u32)) +	/* ETHTOOL_A_RCLK_PIN_FLAGS */
+		       nla_total_size(sizeof(u32));	/* ETHTOOL_A_RCLK_OUT_PIN_IDX */
+	else
+		return nla_total_size(sizeof(u32)) +	/* ETHTOOL_A_RCLK_PIN_MIN */
+		       nla_total_size(sizeof(u32));	/* ETHTOOL_A_RCLK_PIN_MAX */
+}
+
+const struct ethnl_request_ops ethnl_rclk_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_RCLK_GET,
+	.reply_cmd		= ETHTOOL_MSG_RCLK_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_RCLK_HEADER,
+	.req_info_size		= sizeof(struct rclk_request_data),
+	.reply_data_size	= sizeof(struct rclk_reply_data),
+
+	.parse_request		= rclk_parse_request,
+	.prepare_data		= rclk_prepare_data,
+	.reply_size		= rclk_reply_size,
+	.fill_reply		= rclk_fill_reply,
+};
+
+/* RCLK SET */
+
+const struct nla_policy
+ethnl_rclk_set_policy[ETHTOOL_A_RCLK_PIN_FLAGS + 1] = {
+	[ETHTOOL_A_RCLK_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_RCLK_OUT_PIN_IDX] = { .type = NLA_U32 },
+	[ETHTOOL_A_RCLK_PIN_FLAGS] = { .type = NLA_U32 },
+};
+
+static int rclk_set_state(struct net_device *dev, struct nlattr **tb,
+			  bool *p_mod, struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	bool old_state, new_state;
+	u32 min_idx, max_idx;
+	u32 out_idx;
+	int ret;
+
+	if (!tb[ETHTOOL_A_RCLK_PIN_FLAGS] &&
+	    !tb[ETHTOOL_A_RCLK_OUT_PIN_IDX])
+		return 0;
+
+	if (!ops->set_rclk_out || !ops->get_rclk_range) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[ETHTOOL_A_RCLK_PIN_FLAGS],
+				    "Setting recovered clock state is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	ret = ops->get_rclk_range(dev, &min_idx, &max_idx, extack);
+	if (ret)
+		return ret;
+
+	out_idx = nla_get_u32(tb[ETHTOOL_A_RCLK_OUT_PIN_IDX]);
+	if (out_idx < min_idx || out_idx > max_idx) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[ETHTOOL_A_RCLK_OUT_PIN_IDX],
+				    "Requested recovered clock pin index is out of range");
+		return -EINVAL;
+	}
+
+	ret = ops->get_rclk_state(dev, out_idx, &old_state, extack);
+	if (ret < 0)
+		return ret;
+
+	new_state = !!(nla_get_u32(tb[ETHTOOL_A_RCLK_PIN_FLAGS]) &
+		       ETHTOOL_RCLK_PIN_FLAGS_ENA);
+
+	/* If state changed - flag need for sending the notification */
+	*p_mod = old_state != new_state;
+
+	return ops->set_rclk_out(dev, out_idx, new_state, extack);
+}
+
+int ethnl_set_rclk(struct sk_buff *skb, struct genl_info *info)
+{
+	struct ethnl_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct net_device *dev;
+	bool mod = false;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info, tb[ETHTOOL_A_RCLK_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+	dev = req_info.dev;
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = rclk_set_state(dev, tb, &mod, info->extack);
+	if (ret < 0)
+		goto out_ops;
+
+	if (!mod)
+		goto out_ops;
+
+	ethtool_notify(dev, ETHTOOL_MSG_RCLK_NTF, NULL);
+
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+	dev_put(dev);
+	return ret;
+}
+
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v5 net-next 3/4] ice: add support for monitoring SyncE DPLL state
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-10 13:45   ` Maciej Machnikowski
  -1 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: maciej.machnikowski, netdev, intel-wired-lan, arkadiusz.kubalewski
  Cc: richardcochran, abyagowi, anthony.l.nguyen, davem, kuba,
	linux-kselftest, idosch, mkubecek, saeed, michael.chan, petrm

Implement SyncE DPLL monitoring for E810-T devices.
Poll loop will periodically check the state of the DPLL and cache it
in the pf structure. State changes will be logged in the system log.

Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h          |  5 ++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 34 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.c   | 36 ++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  5 +-
 drivers/net/ethernet/intel/ice/ice_devids.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_ptp.c      | 35 ++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 48 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   | 34 +++++++++++++
 8 files changed, 199 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index cb6b4c53584b..2dcc8fd6dff5 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -607,6 +607,11 @@ struct ice_pf {
 #define ICE_VF_AGG_NODE_ID_START	65
 #define ICE_MAX_VF_AGG_NODES		32
 	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
+
+	enum ice_eec_state synce_dpll_state;
+	u8 synce_dpll_pin;
+	enum ice_eec_state ptp_dpll_state;
+	u8 ptp_dpll_pin;
 };
 
 struct ice_netdev_priv {
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 339c2a86f680..11226af7a9a4 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1808,6 +1808,36 @@ struct ice_aqc_add_rdma_qset_data {
 	struct ice_aqc_add_tx_rdma_qset_entry rdma_qsets[];
 };
 
+/* Get CGU DPLL status (direct 0x0C66) */
+struct ice_aqc_get_cgu_dpll_status {
+	u8 dpll_num;
+	u8 ref_state;
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_LOS		BIT(0)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_SCM		BIT(1)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_CFM		BIT(2)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_GST		BIT(3)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_PFM		BIT(4)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_ESYNC	BIT(6)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_FAST_LOCK_EN	BIT(7)
+	__le16 dpll_state;
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_LOCK		BIT(0)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO		BIT(1)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY	BIT(2)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_FLHIT		BIT(5)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_PSLHIT	BIT(7)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT	8
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SEL	\
+	ICE_M(0x1F, ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE_SHIFT	13
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE \
+	ICE_M(0x7, ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE_SHIFT)
+	__le32 phase_offset_h;
+	__le32 phase_offset_l;
+	u8 eec_mode;
+	u8 rsvd[1];
+	__le16 node_handle;
+};
+
 /* Configure Firmware Logging Command (indirect 0xFF09)
  * Logging Information Read Response (indirect 0xFF10)
  * Note: The 0xFF10 command has no input parameters.
@@ -2039,6 +2069,7 @@ struct ice_aq_desc {
 		struct ice_aqc_fw_logging fw_logging;
 		struct ice_aqc_get_clear_fw_log get_clear_fw_log;
 		struct ice_aqc_download_pkg download_pkg;
+		struct ice_aqc_get_cgu_dpll_status get_cgu_dpll_status;
 		struct ice_aqc_driver_shared_params drv_shared_params;
 		struct ice_aqc_set_mac_lb set_mac_lb;
 		struct ice_aqc_alloc_free_res_cmd sw_res_ctrl;
@@ -2205,6 +2236,9 @@ enum ice_adminq_opc {
 	ice_aqc_opc_update_pkg				= 0x0C42,
 	ice_aqc_opc_get_pkg_info_list			= 0x0C43,
 
+	/* 1588/SyncE commands/events */
+	ice_aqc_opc_get_cgu_dpll_status			= 0x0C66,
+
 	ice_aqc_opc_driver_shared_params		= 0x0C90,
 
 	/* Standalone Commands/Events */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 35903b282885..8069141ac105 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -4644,6 +4644,42 @@ ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 	return ice_status_to_errno(status);
 }
 
+/**
+ * ice_aq_get_cgu_dpll_status
+ * @hw: pointer to the HW struct
+ * @dpll_num: DPLL index
+ * @ref_state: Reference clock state
+ * @dpll_state: DPLL state
+ * @phase_offset: Phase offset in ps
+ * @eec_mode: EEC_mode
+ *
+ * Get CGU DPLL status (0x0C66)
+ */
+enum ice_status
+ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
+			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode)
+{
+	struct ice_aqc_get_cgu_dpll_status *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_cgu_dpll_status);
+	cmd = &desc.params.get_cgu_dpll_status;
+	cmd->dpll_num = dpll_num;
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status) {
+		*ref_state = cmd->ref_state;
+		*dpll_state = le16_to_cpu(cmd->dpll_state);
+		*phase_offset = le32_to_cpu(cmd->phase_offset_h);
+		*phase_offset <<= 32;
+		*phase_offset += le32_to_cpu(cmd->phase_offset_l);
+		*eec_mode = cmd->eec_mode;
+	}
+
+	return status;
+}
+
 /**
  * ice_replay_pre_init - replay pre initialization
  * @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 b20a5c085246..aaed388a40a8 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -106,6 +106,7 @@ enum ice_status
 ice_aq_manage_mac_write(struct ice_hw *hw, const u8 *mac_addr, u8 flags,
 			struct ice_sq_cd *cd);
 bool ice_is_e810(struct ice_hw *hw);
+bool ice_is_e810t(struct ice_hw *hw);
 enum ice_status ice_clear_pf_cfg(struct ice_hw *hw);
 enum ice_status
 ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi,
@@ -162,6 +163,9 @@ ice_cfg_vsi_rdma(struct ice_port_info *pi, u16 vsi_handle, u16 tc_bitmap,
 int
 ice_ena_vsi_rdma_qset(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
 		      u16 *rdma_qset, u16 num_qsets, u32 *qset_teid);
+enum ice_status
+ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
+			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode);
 int
 ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 		      u16 *q_id);
@@ -189,7 +193,6 @@ ice_stat_update40(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
 void
 ice_stat_update32(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
 		  u64 *prev_stat, u64 *cur_stat);
-bool ice_is_e810t(struct ice_hw *hw);
 enum ice_status
 ice_sched_query_elem(struct ice_hw *hw, u32 node_teid,
 		     struct ice_aqc_txsched_elem_data *buf);
diff --git a/drivers/net/ethernet/intel/ice/ice_devids.h b/drivers/net/ethernet/intel/ice/ice_devids.h
index 61dd2f18dee8..0b654d417d29 100644
--- a/drivers/net/ethernet/intel/ice/ice_devids.h
+++ b/drivers/net/ethernet/intel/ice/ice_devids.h
@@ -58,4 +58,7 @@
 /* Intel(R) Ethernet Connection E822-L 1GbE */
 #define ICE_DEV_ID_E822L_SGMII		0x189A
 
+#define ICE_SUBDEV_ID_E810T		0x000E
+#define ICE_SUBDEV_ID_E810T2		0x000F
+
 #endif /* _ICE_DEVIDS_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index bf7247c6f58e..bb502c19d53a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1766,6 +1766,36 @@ static void ice_ptp_tx_tstamp_cleanup(struct ice_ptp_tx *tx)
 	}
 }
 
+static void ice_handle_cgu_state(struct ice_pf *pf)
+{
+	enum ice_eec_state cgu_state;
+	u8 pin;
+
+	cgu_state = ice_get_zl_dpll_state(&pf->hw, ICE_CGU_DPLL_SYNCE, &pin);
+	if (pf->synce_dpll_state != cgu_state) {
+		pf->synce_dpll_state = cgu_state;
+		pf->synce_dpll_pin = pin;
+
+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_SYNCE,
+			 pf->synce_dpll_state,
+			 pin);
+	}
+
+	cgu_state = ice_get_zl_dpll_state(&pf->hw, ICE_CGU_DPLL_PTP, &pin);
+	if (pf->ptp_dpll_state != cgu_state) {
+		pf->ptp_dpll_state = cgu_state;
+		pf->ptp_dpll_pin = pin;
+
+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_PTP,
+			 pf->ptp_dpll_state,
+			 pin);
+	}
+}
+
 static void ice_ptp_periodic_work(struct kthread_work *work)
 {
 	struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
@@ -1774,6 +1804,10 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
 	if (!test_bit(ICE_FLAG_PTP, pf->flags))
 		return;
 
+	if (ice_is_feature_supported(pf, ICE_F_CGU) &&
+	    pf->hw.func_caps.ts_func_info.src_tmr_owned)
+		ice_handle_cgu_state(pf);
+
 	ice_ptp_update_cached_phctime(pf);
 
 	ice_ptp_tx_tstamp_cleanup(&pf->ptp.port.tx);
@@ -1958,3 +1992,4 @@ void ice_ptp_release(struct ice_pf *pf)
 
 	dev_info(ice_pf_to_dev(pf), "Removed PTP clock\n");
 }
+
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index aa257db36765..b4300bf3e4ce 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -375,6 +375,54 @@ static int ice_ptp_port_cmd_e810(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
 	return 0;
 }
 
+/**
+ * ice_get_zl_dpll_state - get the state of the DPLL
+ * @hw: pointer to the hw struct
+ * @dpll_idx: Index of internal DPLL unit
+ * @pin: pointer to a buffer for returning currently active pin
+ *
+ * This function will read the state of the DPLL(dpll_idx). If optional
+ * parameter pin is given it'll be used to retrieve currently active pin.
+ *
+ * Return: state of the DPLL
+ */
+enum ice_eec_state
+ice_get_zl_dpll_state(struct ice_hw *hw, u8 dpll_idx, u8 *pin)
+{
+	enum ice_status status;
+	u64 phase_offset;
+	u16 dpll_state;
+	u8 ref_state;
+	u8 eec_mode;
+
+	if (dpll_idx >= ICE_CGU_DPLL_MAX)
+		return ICE_EEC_STATE_INVALID;
+
+	status = ice_aq_get_cgu_dpll_status(hw, dpll_idx, &ref_state,
+					    &dpll_state, &phase_offset,
+					    &eec_mode);
+	if (status)
+		return ICE_EEC_STATE_INVALID;
+
+	if (pin) {
+		/* current ref pin in dpll_state_refsel_status_X register */
+		*pin = (dpll_state &
+			ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SEL) >>
+		       ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT;
+	}
+
+	if (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_LOCK) {
+		if (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY)
+			return ICE_EEC_STATE_LOCKED_HO_ACQ;
+		else
+			return ICE_EEC_STATE_LOCKED;
+	} else if ((dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO) &&
+		  (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY)) {
+		return ICE_EEC_STATE_HOLDOVER;
+	}
+	return ICE_EEC_STATE_FREERUN;
+}
+
 /* Device agnostic functions
  *
  * The following functions implement useful behavior to hide the differences
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index b2984b5c22c1..28b04ec40bae 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -12,6 +12,18 @@ enum ice_ptp_tmr_cmd {
 	READ_TIME
 };
 
+enum ice_eec_state {
+	ICE_EEC_STATE_INVALID = 0,       /* state is not valid */
+	ICE_EEC_STATE_FREERUN,           /* clock is free-running */
+	ICE_EEC_STATE_LOCKED,            /* clock is locked to the reference,
+					  * but the holdover memory is not valid
+					  */
+	ICE_EEC_STATE_LOCKED_HO_ACQ,     /* clock is locked to the reference
+					  * and holdover memory is valid
+					  */
+	ICE_EEC_STATE_HOLDOVER,          /* clock is in holdover mode */
+};
+
 /* Increment value to generate nanoseconds in the GLTSYN_TIME_L register for
  * the E810 devices. Based off of a PLL with an 812.5 MHz frequency.
  */
@@ -33,6 +45,8 @@ int ice_ptp_init_phy_e810(struct ice_hw *hw);
 int ice_read_sma_ctrl_e810t(struct ice_hw *hw, u8 *data);
 int ice_write_sma_ctrl_e810t(struct ice_hw *hw, u8 data);
 bool ice_is_pca9575_present(struct ice_hw *hw);
+enum ice_eec_state
+ice_get_zl_dpll_state(struct ice_hw *hw, u8 dpll_idx, u8 *pin);
 
 #define PFTSYN_SEM_BYTES	4
 
@@ -98,4 +112,24 @@ bool ice_is_pca9575_present(struct ice_hw *hw);
 #define ICE_SMA_MAX_BIT_E810T	7
 #define ICE_PCA9575_P1_OFFSET	8
 
+enum ice_e810t_cgu_dpll {
+	ICE_CGU_DPLL_SYNCE,
+	ICE_CGU_DPLL_PTP,
+	ICE_CGU_DPLL_MAX
+};
+
+enum ice_e810t_cgu_pins {
+	REF0P,
+	REF0N,
+	REF1P,
+	REF1N,
+	REF2P,
+	REF2N,
+	REF3P,
+	REF3N,
+	REF4P,
+	REF4N,
+	NUM_E810T_CGU_PINS
+};
+
 #endif /* _ICE_PTP_HW_H_ */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 3/4] ice: add support for monitoring SyncE DPLL state
@ 2021-12-10 13:45   ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: intel-wired-lan

Implement SyncE DPLL monitoring for E810-T devices.
Poll loop will periodically check the state of the DPLL and cache it
in the pf structure. State changes will be logged in the system log.

Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h          |  5 ++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 34 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.c   | 36 ++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  5 +-
 drivers/net/ethernet/intel/ice/ice_devids.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_ptp.c      | 35 ++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 48 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   | 34 +++++++++++++
 8 files changed, 199 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index cb6b4c53584b..2dcc8fd6dff5 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -607,6 +607,11 @@ struct ice_pf {
 #define ICE_VF_AGG_NODE_ID_START	65
 #define ICE_MAX_VF_AGG_NODES		32
 	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
+
+	enum ice_eec_state synce_dpll_state;
+	u8 synce_dpll_pin;
+	enum ice_eec_state ptp_dpll_state;
+	u8 ptp_dpll_pin;
 };
 
 struct ice_netdev_priv {
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 339c2a86f680..11226af7a9a4 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1808,6 +1808,36 @@ struct ice_aqc_add_rdma_qset_data {
 	struct ice_aqc_add_tx_rdma_qset_entry rdma_qsets[];
 };
 
+/* Get CGU DPLL status (direct 0x0C66) */
+struct ice_aqc_get_cgu_dpll_status {
+	u8 dpll_num;
+	u8 ref_state;
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_LOS		BIT(0)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_SCM		BIT(1)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_CFM		BIT(2)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_GST		BIT(3)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_PFM		BIT(4)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_REF_SW_ESYNC	BIT(6)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_FAST_LOCK_EN	BIT(7)
+	__le16 dpll_state;
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_LOCK		BIT(0)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO		BIT(1)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY	BIT(2)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_FLHIT		BIT(5)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_PSLHIT	BIT(7)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT	8
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SEL	\
+	ICE_M(0x1F, ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT)
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE_SHIFT	13
+#define ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE \
+	ICE_M(0x7, ICE_AQC_GET_CGU_DPLL_STATUS_STATE_MODE_SHIFT)
+	__le32 phase_offset_h;
+	__le32 phase_offset_l;
+	u8 eec_mode;
+	u8 rsvd[1];
+	__le16 node_handle;
+};
+
 /* Configure Firmware Logging Command (indirect 0xFF09)
  * Logging Information Read Response (indirect 0xFF10)
  * Note: The 0xFF10 command has no input parameters.
@@ -2039,6 +2069,7 @@ struct ice_aq_desc {
 		struct ice_aqc_fw_logging fw_logging;
 		struct ice_aqc_get_clear_fw_log get_clear_fw_log;
 		struct ice_aqc_download_pkg download_pkg;
+		struct ice_aqc_get_cgu_dpll_status get_cgu_dpll_status;
 		struct ice_aqc_driver_shared_params drv_shared_params;
 		struct ice_aqc_set_mac_lb set_mac_lb;
 		struct ice_aqc_alloc_free_res_cmd sw_res_ctrl;
@@ -2205,6 +2236,9 @@ enum ice_adminq_opc {
 	ice_aqc_opc_update_pkg				= 0x0C42,
 	ice_aqc_opc_get_pkg_info_list			= 0x0C43,
 
+	/* 1588/SyncE commands/events */
+	ice_aqc_opc_get_cgu_dpll_status			= 0x0C66,
+
 	ice_aqc_opc_driver_shared_params		= 0x0C90,
 
 	/* Standalone Commands/Events */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 35903b282885..8069141ac105 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -4644,6 +4644,42 @@ ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 	return ice_status_to_errno(status);
 }
 
+/**
+ * ice_aq_get_cgu_dpll_status
+ * @hw: pointer to the HW struct
+ * @dpll_num: DPLL index
+ * @ref_state: Reference clock state
+ * @dpll_state: DPLL state
+ * @phase_offset: Phase offset in ps
+ * @eec_mode: EEC_mode
+ *
+ * Get CGU DPLL status (0x0C66)
+ */
+enum ice_status
+ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
+			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode)
+{
+	struct ice_aqc_get_cgu_dpll_status *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_cgu_dpll_status);
+	cmd = &desc.params.get_cgu_dpll_status;
+	cmd->dpll_num = dpll_num;
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status) {
+		*ref_state = cmd->ref_state;
+		*dpll_state = le16_to_cpu(cmd->dpll_state);
+		*phase_offset = le32_to_cpu(cmd->phase_offset_h);
+		*phase_offset <<= 32;
+		*phase_offset += le32_to_cpu(cmd->phase_offset_l);
+		*eec_mode = cmd->eec_mode;
+	}
+
+	return status;
+}
+
 /**
  * ice_replay_pre_init - replay pre initialization
  * @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 b20a5c085246..aaed388a40a8 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -106,6 +106,7 @@ enum ice_status
 ice_aq_manage_mac_write(struct ice_hw *hw, const u8 *mac_addr, u8 flags,
 			struct ice_sq_cd *cd);
 bool ice_is_e810(struct ice_hw *hw);
+bool ice_is_e810t(struct ice_hw *hw);
 enum ice_status ice_clear_pf_cfg(struct ice_hw *hw);
 enum ice_status
 ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi,
@@ -162,6 +163,9 @@ ice_cfg_vsi_rdma(struct ice_port_info *pi, u16 vsi_handle, u16 tc_bitmap,
 int
 ice_ena_vsi_rdma_qset(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
 		      u16 *rdma_qset, u16 num_qsets, u32 *qset_teid);
+enum ice_status
+ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
+			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode);
 int
 ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 		      u16 *q_id);
@@ -189,7 +193,6 @@ ice_stat_update40(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
 void
 ice_stat_update32(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
 		  u64 *prev_stat, u64 *cur_stat);
-bool ice_is_e810t(struct ice_hw *hw);
 enum ice_status
 ice_sched_query_elem(struct ice_hw *hw, u32 node_teid,
 		     struct ice_aqc_txsched_elem_data *buf);
diff --git a/drivers/net/ethernet/intel/ice/ice_devids.h b/drivers/net/ethernet/intel/ice/ice_devids.h
index 61dd2f18dee8..0b654d417d29 100644
--- a/drivers/net/ethernet/intel/ice/ice_devids.h
+++ b/drivers/net/ethernet/intel/ice/ice_devids.h
@@ -58,4 +58,7 @@
 /* Intel(R) Ethernet Connection E822-L 1GbE */
 #define ICE_DEV_ID_E822L_SGMII		0x189A
 
+#define ICE_SUBDEV_ID_E810T		0x000E
+#define ICE_SUBDEV_ID_E810T2		0x000F
+
 #endif /* _ICE_DEVIDS_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index bf7247c6f58e..bb502c19d53a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1766,6 +1766,36 @@ static void ice_ptp_tx_tstamp_cleanup(struct ice_ptp_tx *tx)
 	}
 }
 
+static void ice_handle_cgu_state(struct ice_pf *pf)
+{
+	enum ice_eec_state cgu_state;
+	u8 pin;
+
+	cgu_state = ice_get_zl_dpll_state(&pf->hw, ICE_CGU_DPLL_SYNCE, &pin);
+	if (pf->synce_dpll_state != cgu_state) {
+		pf->synce_dpll_state = cgu_state;
+		pf->synce_dpll_pin = pin;
+
+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_SYNCE,
+			 pf->synce_dpll_state,
+			 pin);
+	}
+
+	cgu_state = ice_get_zl_dpll_state(&pf->hw, ICE_CGU_DPLL_PTP, &pin);
+	if (pf->ptp_dpll_state != cgu_state) {
+		pf->ptp_dpll_state = cgu_state;
+		pf->ptp_dpll_pin = pin;
+
+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_PTP,
+			 pf->ptp_dpll_state,
+			 pin);
+	}
+}
+
 static void ice_ptp_periodic_work(struct kthread_work *work)
 {
 	struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
@@ -1774,6 +1804,10 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
 	if (!test_bit(ICE_FLAG_PTP, pf->flags))
 		return;
 
+	if (ice_is_feature_supported(pf, ICE_F_CGU) &&
+	    pf->hw.func_caps.ts_func_info.src_tmr_owned)
+		ice_handle_cgu_state(pf);
+
 	ice_ptp_update_cached_phctime(pf);
 
 	ice_ptp_tx_tstamp_cleanup(&pf->ptp.port.tx);
@@ -1958,3 +1992,4 @@ void ice_ptp_release(struct ice_pf *pf)
 
 	dev_info(ice_pf_to_dev(pf), "Removed PTP clock\n");
 }
+
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index aa257db36765..b4300bf3e4ce 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -375,6 +375,54 @@ static int ice_ptp_port_cmd_e810(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
 	return 0;
 }
 
+/**
+ * ice_get_zl_dpll_state - get the state of the DPLL
+ * @hw: pointer to the hw struct
+ * @dpll_idx: Index of internal DPLL unit
+ * @pin: pointer to a buffer for returning currently active pin
+ *
+ * This function will read the state of the DPLL(dpll_idx). If optional
+ * parameter pin is given it'll be used to retrieve currently active pin.
+ *
+ * Return: state of the DPLL
+ */
+enum ice_eec_state
+ice_get_zl_dpll_state(struct ice_hw *hw, u8 dpll_idx, u8 *pin)
+{
+	enum ice_status status;
+	u64 phase_offset;
+	u16 dpll_state;
+	u8 ref_state;
+	u8 eec_mode;
+
+	if (dpll_idx >= ICE_CGU_DPLL_MAX)
+		return ICE_EEC_STATE_INVALID;
+
+	status = ice_aq_get_cgu_dpll_status(hw, dpll_idx, &ref_state,
+					    &dpll_state, &phase_offset,
+					    &eec_mode);
+	if (status)
+		return ICE_EEC_STATE_INVALID;
+
+	if (pin) {
+		/* current ref pin in dpll_state_refsel_status_X register */
+		*pin = (dpll_state &
+			ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SEL) >>
+		       ICE_AQC_GET_CGU_DPLL_STATUS_STATE_CLK_REF_SHIFT;
+	}
+
+	if (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_LOCK) {
+		if (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY)
+			return ICE_EEC_STATE_LOCKED_HO_ACQ;
+		else
+			return ICE_EEC_STATE_LOCKED;
+	} else if ((dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO) &&
+		  (dpll_state & ICE_AQC_GET_CGU_DPLL_STATUS_STATE_HO_READY)) {
+		return ICE_EEC_STATE_HOLDOVER;
+	}
+	return ICE_EEC_STATE_FREERUN;
+}
+
 /* Device agnostic functions
  *
  * The following functions implement useful behavior to hide the differences
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index b2984b5c22c1..28b04ec40bae 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -12,6 +12,18 @@ enum ice_ptp_tmr_cmd {
 	READ_TIME
 };
 
+enum ice_eec_state {
+	ICE_EEC_STATE_INVALID = 0,       /* state is not valid */
+	ICE_EEC_STATE_FREERUN,           /* clock is free-running */
+	ICE_EEC_STATE_LOCKED,            /* clock is locked to the reference,
+					  * but the holdover memory is not valid
+					  */
+	ICE_EEC_STATE_LOCKED_HO_ACQ,     /* clock is locked to the reference
+					  * and holdover memory is valid
+					  */
+	ICE_EEC_STATE_HOLDOVER,          /* clock is in holdover mode */
+};
+
 /* Increment value to generate nanoseconds in the GLTSYN_TIME_L register for
  * the E810 devices. Based off of a PLL with an 812.5 MHz frequency.
  */
@@ -33,6 +45,8 @@ int ice_ptp_init_phy_e810(struct ice_hw *hw);
 int ice_read_sma_ctrl_e810t(struct ice_hw *hw, u8 *data);
 int ice_write_sma_ctrl_e810t(struct ice_hw *hw, u8 data);
 bool ice_is_pca9575_present(struct ice_hw *hw);
+enum ice_eec_state
+ice_get_zl_dpll_state(struct ice_hw *hw, u8 dpll_idx, u8 *pin);
 
 #define PFTSYN_SEM_BYTES	4
 
@@ -98,4 +112,24 @@ bool ice_is_pca9575_present(struct ice_hw *hw);
 #define ICE_SMA_MAX_BIT_E810T	7
 #define ICE_PCA9575_P1_OFFSET	8
 
+enum ice_e810t_cgu_dpll {
+	ICE_CGU_DPLL_SYNCE,
+	ICE_CGU_DPLL_PTP,
+	ICE_CGU_DPLL_MAX
+};
+
+enum ice_e810t_cgu_pins {
+	REF0P,
+	REF0N,
+	REF1P,
+	REF1N,
+	REF2P,
+	REF2N,
+	REF3P,
+	REF3N,
+	REF4P,
+	REF4N,
+	NUM_E810T_CGU_PINS
+};
+
 #endif /* _ICE_PTP_HW_H_ */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v5 net-next 4/4] ice: add support for recovered clocks
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-10 13:45   ` Maciej Machnikowski
  -1 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: maciej.machnikowski, netdev, intel-wired-lan, arkadiusz.kubalewski
  Cc: richardcochran, abyagowi, anthony.l.nguyen, davem, kuba,
	linux-kselftest, idosch, mkubecek, saeed, michael.chan, petrm

Implement ethtool netlink functions for handling recovered clocks
configuration on ice driver:
- ETHTOOL_MSG_RCLK_SET
- ETHTOOL_MSG_RCLK_GET

Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 29 ++++++
 drivers/net/ethernet/intel/ice/ice_common.c   | 65 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  6 ++
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 96 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  2 +
 5 files changed, 198 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 11226af7a9a4..aed03200bb99 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1281,6 +1281,31 @@ struct ice_aqc_set_mac_lb {
 	u8 reserved[15];
 };
 
+/* Set PHY recovered clock output (direct 0x0630) */
+struct ice_aqc_set_phy_rec_clk_out {
+	u8 phy_output;
+	u8 port_num;
+	u8 flags;
+#define ICE_AQC_SET_PHY_REC_CLK_OUT_OUT_EN	BIT(0)
+#define ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT	0xFF
+	u8 rsvd;
+	__le32 freq;
+	u8 rsvd2[6];
+	__le16 node_handle;
+};
+
+/* Get PHY recovered clock output (direct 0x0631) */
+struct ice_aqc_get_phy_rec_clk_out {
+	u8 phy_output;
+	u8 port_num;
+	u8 flags;
+#define ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN	BIT(0)
+	u8 rsvd;
+	__le32 freq;
+	u8 rsvd2[6];
+	__le16 node_handle;
+};
+
 struct ice_aqc_link_topo_params {
 	u8 lport_num;
 	u8 lport_num_valid;
@@ -2033,6 +2058,8 @@ struct ice_aq_desc {
 		struct ice_aqc_get_phy_caps get_phy;
 		struct ice_aqc_set_phy_cfg set_phy;
 		struct ice_aqc_restart_an restart_an;
+		struct ice_aqc_set_phy_rec_clk_out set_phy_rec_clk_out;
+		struct ice_aqc_get_phy_rec_clk_out get_phy_rec_clk_out;
 		struct ice_aqc_gpio read_write_gpio;
 		struct ice_aqc_sff_eeprom read_write_sff_param;
 		struct ice_aqc_set_port_id_led set_port_id_led;
@@ -2188,6 +2215,8 @@ enum ice_adminq_opc {
 	ice_aqc_opc_get_link_status			= 0x0607,
 	ice_aqc_opc_set_event_mask			= 0x0613,
 	ice_aqc_opc_set_mac_lb				= 0x0620,
+	ice_aqc_opc_set_phy_rec_clk_out			= 0x0630,
+	ice_aqc_opc_get_phy_rec_clk_out			= 0x0631,
 	ice_aqc_opc_get_link_topo			= 0x06E0,
 	ice_aqc_opc_set_port_id_led			= 0x06E9,
 	ice_aqc_opc_set_gpio				= 0x06EC,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 8069141ac105..29d302ea1e56 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -5242,3 +5242,68 @@ bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
 	return true;
 }
 
+/**
+ * ice_aq_set_phy_rec_clk_out - set RCLK phy out
+ * @hw: pointer to the HW struct
+ * @phy_output: PHY reference clock output pin
+ * @enable: GPIO state to be applied
+ * @freq: PHY output frequency
+ *
+ * Set CGU reference priority (0x0630)
+ * Return 0 on success or negative value on failure.
+ */
+enum ice_status
+ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, bool enable,
+			   u32 *freq)
+{
+	struct ice_aqc_set_phy_rec_clk_out *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_set_phy_rec_clk_out);
+	cmd = &desc.params.set_phy_rec_clk_out;
+	cmd->phy_output = phy_output;
+	cmd->port_num = ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT;
+	cmd->flags = enable & ICE_AQC_SET_PHY_REC_CLK_OUT_OUT_EN;
+	cmd->freq = cpu_to_le32(*freq);
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status)
+		*freq = le32_to_cpu(cmd->freq);
+
+	return status;
+}
+
+/**
+ * ice_aq_get_phy_rec_clk_out
+ * @hw: pointer to the HW struct
+ * @phy_output: PHY reference clock output pin
+ * @port_num: Port number
+ * @flags: PHY flags
+ * @freq: PHY output frequency
+ *
+ * Get PHY recovered clock output (0x0631)
+ */
+enum ice_status
+ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, u8 *port_num,
+			   u8 *flags, u32 *freq)
+{
+	struct ice_aqc_get_phy_rec_clk_out *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_phy_rec_clk_out);
+	cmd = &desc.params.get_phy_rec_clk_out;
+	cmd->phy_output = phy_output;
+	cmd->port_num = *port_num;
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status) {
+		*port_num = cmd->port_num;
+		*flags = cmd->flags;
+		*freq = le32_to_cpu(cmd->freq);
+	}
+
+	return status;
+}
+
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index aaed388a40a8..8a99c8364173 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -166,6 +166,12 @@ ice_ena_vsi_rdma_qset(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
 enum ice_status
 ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
 			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode);
+enum ice_status
+ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, bool enable,
+			   u32 *freq);
+enum ice_status
+ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, u8 *port_num,
+			   u8 *flags, u32 *freq);
 int
 ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 		      u16 *q_id);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 5af2faaa21e1..2ae58f23c845 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4076,6 +4076,99 @@ ice_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
+/**
+ * ice_get_rclk_range - get range of recovered clock indices
+ * @netdev: network interface device structure
+ * @min_idx: min rclk index
+ * @max_idx: max rclk index
+ * @extack: netlink extended ack
+ */
+static int
+ice_get_rclk_range(struct net_device *netdev, u32 *min_idx, u32 *max_idx,
+		   struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	*min_idx = 0;
+	*max_idx = ICE_RCLK_PIN_MAX;
+
+	return 0;
+}
+
+/**
+ * ice_get_rclk_state - get state of a recovered frequency output pin
+ * @netdev: network interface device structure
+ * @out_idx: index of a questioned pin
+ * @ena: returned state of a pin
+ * @extack: netlink extended ack
+ */
+static int
+ice_get_rclk_state(struct net_device *netdev, u32 out_idx,
+		   bool *ena, struct netlink_ext_ack *extack)
+{
+	u8 port_num = ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT, flags;
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	u32 freq;
+	int ret;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	if (out_idx > ICE_RCLK_PIN_MAX)
+		return -EINVAL;
+
+	ret = ice_aq_get_phy_rec_clk_out(&pf->hw, out_idx,
+					 &port_num, &flags, &freq);
+	if (ret)
+		return ret;
+
+	if (flags & ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN)
+		*ena = true;
+	else
+		*ena = false;
+
+	return ret;
+}
+
+/**
+ * ice_set_rclk_out - enable/disable recovered clock redirection to the
+ * output pin
+ * @netdev: network interface device structure
+ * @out_idx: index of pin being configured
+ * @ena: requested state of a pin
+ * @extack: netlink extended ack
+ */
+static int
+ice_set_rclk_out(struct net_device *netdev, u32 out_idx, bool ena,
+		 struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	enum ice_status ret;
+	u32 freq;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	if (out_idx > ICE_RCLK_PIN_MAX)
+		return -EINVAL;
+
+	ret = ice_aq_set_phy_rec_clk_out(&pf->hw, out_idx,
+					 ena, &freq);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
 static const struct ethtool_ops ice_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE |
@@ -4121,6 +4214,9 @@ static const struct ethtool_ops ice_ethtool_ops = {
 	.set_fecparam		= ice_set_fecparam,
 	.get_module_info	= ice_get_module_info,
 	.get_module_eeprom	= ice_get_module_eeprom,
+	.get_rclk_range		= ice_get_rclk_range,
+	.get_rclk_state		= ice_get_rclk_state,
+	.set_rclk_out		= ice_set_rclk_out,
 };
 
 static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 28b04ec40bae..865ca680b62e 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -132,4 +132,6 @@ enum ice_e810t_cgu_pins {
 	NUM_E810T_CGU_PINS
 };
 
+#define ICE_RCLK_PIN_MAX	(REF1N - REF1P)
+
 #endif /* _ICE_PTP_HW_H_ */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 4/4] ice: add support for recovered clocks
@ 2021-12-10 13:45   ` Maciej Machnikowski
  0 siblings, 0 replies; 30+ messages in thread
From: Maciej Machnikowski @ 2021-12-10 13:45 UTC (permalink / raw)
  To: intel-wired-lan

Implement ethtool netlink functions for handling recovered clocks
configuration on ice driver:
- ETHTOOL_MSG_RCLK_SET
- ETHTOOL_MSG_RCLK_GET

Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 29 ++++++
 drivers/net/ethernet/intel/ice/ice_common.c   | 65 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  6 ++
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 96 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  2 +
 5 files changed, 198 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 11226af7a9a4..aed03200bb99 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1281,6 +1281,31 @@ struct ice_aqc_set_mac_lb {
 	u8 reserved[15];
 };
 
+/* Set PHY recovered clock output (direct 0x0630) */
+struct ice_aqc_set_phy_rec_clk_out {
+	u8 phy_output;
+	u8 port_num;
+	u8 flags;
+#define ICE_AQC_SET_PHY_REC_CLK_OUT_OUT_EN	BIT(0)
+#define ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT	0xFF
+	u8 rsvd;
+	__le32 freq;
+	u8 rsvd2[6];
+	__le16 node_handle;
+};
+
+/* Get PHY recovered clock output (direct 0x0631) */
+struct ice_aqc_get_phy_rec_clk_out {
+	u8 phy_output;
+	u8 port_num;
+	u8 flags;
+#define ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN	BIT(0)
+	u8 rsvd;
+	__le32 freq;
+	u8 rsvd2[6];
+	__le16 node_handle;
+};
+
 struct ice_aqc_link_topo_params {
 	u8 lport_num;
 	u8 lport_num_valid;
@@ -2033,6 +2058,8 @@ struct ice_aq_desc {
 		struct ice_aqc_get_phy_caps get_phy;
 		struct ice_aqc_set_phy_cfg set_phy;
 		struct ice_aqc_restart_an restart_an;
+		struct ice_aqc_set_phy_rec_clk_out set_phy_rec_clk_out;
+		struct ice_aqc_get_phy_rec_clk_out get_phy_rec_clk_out;
 		struct ice_aqc_gpio read_write_gpio;
 		struct ice_aqc_sff_eeprom read_write_sff_param;
 		struct ice_aqc_set_port_id_led set_port_id_led;
@@ -2188,6 +2215,8 @@ enum ice_adminq_opc {
 	ice_aqc_opc_get_link_status			= 0x0607,
 	ice_aqc_opc_set_event_mask			= 0x0613,
 	ice_aqc_opc_set_mac_lb				= 0x0620,
+	ice_aqc_opc_set_phy_rec_clk_out			= 0x0630,
+	ice_aqc_opc_get_phy_rec_clk_out			= 0x0631,
 	ice_aqc_opc_get_link_topo			= 0x06E0,
 	ice_aqc_opc_set_port_id_led			= 0x06E9,
 	ice_aqc_opc_set_gpio				= 0x06EC,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 8069141ac105..29d302ea1e56 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -5242,3 +5242,68 @@ bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
 	return true;
 }
 
+/**
+ * ice_aq_set_phy_rec_clk_out - set RCLK phy out
+ * @hw: pointer to the HW struct
+ * @phy_output: PHY reference clock output pin
+ * @enable: GPIO state to be applied
+ * @freq: PHY output frequency
+ *
+ * Set CGU reference priority (0x0630)
+ * Return 0 on success or negative value on failure.
+ */
+enum ice_status
+ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, bool enable,
+			   u32 *freq)
+{
+	struct ice_aqc_set_phy_rec_clk_out *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_set_phy_rec_clk_out);
+	cmd = &desc.params.set_phy_rec_clk_out;
+	cmd->phy_output = phy_output;
+	cmd->port_num = ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT;
+	cmd->flags = enable & ICE_AQC_SET_PHY_REC_CLK_OUT_OUT_EN;
+	cmd->freq = cpu_to_le32(*freq);
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status)
+		*freq = le32_to_cpu(cmd->freq);
+
+	return status;
+}
+
+/**
+ * ice_aq_get_phy_rec_clk_out
+ * @hw: pointer to the HW struct
+ * @phy_output: PHY reference clock output pin
+ * @port_num: Port number
+ * @flags: PHY flags
+ * @freq: PHY output frequency
+ *
+ * Get PHY recovered clock output (0x0631)
+ */
+enum ice_status
+ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, u8 *port_num,
+			   u8 *flags, u32 *freq)
+{
+	struct ice_aqc_get_phy_rec_clk_out *cmd;
+	struct ice_aq_desc desc;
+	enum ice_status status;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_phy_rec_clk_out);
+	cmd = &desc.params.get_phy_rec_clk_out;
+	cmd->phy_output = phy_output;
+	cmd->port_num = *port_num;
+
+	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+	if (!status) {
+		*port_num = cmd->port_num;
+		*flags = cmd->flags;
+		*freq = le32_to_cpu(cmd->freq);
+	}
+
+	return status;
+}
+
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index aaed388a40a8..8a99c8364173 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -166,6 +166,12 @@ ice_ena_vsi_rdma_qset(struct ice_port_info *pi, u16 vsi_handle, u8 tc,
 enum ice_status
 ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
 			   u16 *dpll_state, u64 *phase_offset, u8 *eec_mode);
+enum ice_status
+ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, bool enable,
+			   u32 *freq);
+enum ice_status
+ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, u8 *port_num,
+			   u8 *flags, u32 *freq);
 int
 ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 		      u16 *q_id);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 5af2faaa21e1..2ae58f23c845 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4076,6 +4076,99 @@ ice_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
+/**
+ * ice_get_rclk_range - get range of recovered clock indices
+ * @netdev: network interface device structure
+ * @min_idx: min rclk index
+ * @max_idx: max rclk index
+ * @extack: netlink extended ack
+ */
+static int
+ice_get_rclk_range(struct net_device *netdev, u32 *min_idx, u32 *max_idx,
+		   struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	*min_idx = 0;
+	*max_idx = ICE_RCLK_PIN_MAX;
+
+	return 0;
+}
+
+/**
+ * ice_get_rclk_state - get state of a recovered frequency output pin
+ * @netdev: network interface device structure
+ * @out_idx: index of a questioned pin
+ * @ena: returned state of a pin
+ * @extack: netlink extended ack
+ */
+static int
+ice_get_rclk_state(struct net_device *netdev, u32 out_idx,
+		   bool *ena, struct netlink_ext_ack *extack)
+{
+	u8 port_num = ICE_AQC_SET_PHY_REC_CLK_OUT_CURR_PORT, flags;
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	u32 freq;
+	int ret;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	if (out_idx > ICE_RCLK_PIN_MAX)
+		return -EINVAL;
+
+	ret = ice_aq_get_phy_rec_clk_out(&pf->hw, out_idx,
+					 &port_num, &flags, &freq);
+	if (ret)
+		return ret;
+
+	if (flags & ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN)
+		*ena = true;
+	else
+		*ena = false;
+
+	return ret;
+}
+
+/**
+ * ice_set_rclk_out - enable/disable recovered clock redirection to the
+ * output pin
+ * @netdev: network interface device structure
+ * @out_idx: index of pin being configured
+ * @ena: requested state of a pin
+ * @extack: netlink extended ack
+ */
+static int
+ice_set_rclk_out(struct net_device *netdev, u32 out_idx, bool ena,
+		 struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	enum ice_status ret;
+	u32 freq;
+
+	if (!ice_is_feature_supported(pf, ICE_F_CGU))
+		return -EOPNOTSUPP;
+
+	if (out_idx > ICE_RCLK_PIN_MAX)
+		return -EINVAL;
+
+	ret = ice_aq_set_phy_rec_clk_out(&pf->hw, out_idx,
+					 ena, &freq);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
 static const struct ethtool_ops ice_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE |
@@ -4121,6 +4214,9 @@ static const struct ethtool_ops ice_ethtool_ops = {
 	.set_fecparam		= ice_set_fecparam,
 	.get_module_info	= ice_get_module_info,
 	.get_module_eeprom	= ice_get_module_eeprom,
+	.get_rclk_range		= ice_get_rclk_range,
+	.get_rclk_state		= ice_get_rclk_state,
+	.set_rclk_out		= ice_set_rclk_out,
 };
 
 static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 28b04ec40bae..865ca680b62e 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -132,4 +132,6 @@ enum ice_e810t_cgu_pins {
 	NUM_E810T_CGU_PINS
 };
 
+#define ICE_RCLK_PIN_MAX	(REF1N - REF1P)
+
 #endif /* _ICE_PTP_HW_H_ */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-10 16:16   ` Jakub Kicinski
  -1 siblings, 0 replies; 30+ messages in thread
From: Jakub Kicinski @ 2021-12-10 16:16 UTC (permalink / raw)
  To: Maciej Machnikowski
  Cc: netdev, intel-wired-lan, arkadiusz.kubalewski, richardcochran,
	abyagowi, anthony.l.nguyen, davem, linux-kselftest, idosch,
	mkubecek, saeed, michael.chan, petrm, Vadim Fedorenko

On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> Synchronous Ethernet networks use a physical layer clock to syntonize
> the frequency across different network elements.
> 
> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> Equipment Clock (EEC) and have the ability to synchronize to reference
> frequency sources.
> 
> This patch series is a prerequisite for EEC object and adds ability
> to enable recovered clocks in the physical layer of the netdev object.
> Recovered clocks can be used as one of the reference signal by the EEC.
> 
> Further work is required to add the DPLL subsystem, link it to the
> netdev object and create API to read the EEC DPLL state.

You missed CCing Vadim. I guess Ccing the right people may be right up
there with naming things as the hardest things in SW development..

Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-10 16:16   ` Jakub Kicinski
  0 siblings, 0 replies; 30+ messages in thread
From: Jakub Kicinski @ 2021-12-10 16:16 UTC (permalink / raw)
  To: intel-wired-lan

On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> Synchronous Ethernet networks use a physical layer clock to syntonize
> the frequency across different network elements.
> 
> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> Equipment Clock (EEC) and have the ability to synchronize to reference
> frequency sources.
> 
> This patch series is a prerequisite for EEC object and adds ability
> to enable recovered clocks in the physical layer of the netdev object.
> Recovered clocks can be used as one of the reference signal by the EEC.
> 
> Further work is required to add the DPLL subsystem, link it to the
> netdev object and create API to read the EEC DPLL state.

You missed CCing Vadim. I guess Ccing the right people may be right up
there with naming things as the hardest things in SW development..

Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
@ 2021-12-12 11:47   ` Ido Schimmel
  -1 siblings, 0 replies; 30+ messages in thread
From: Ido Schimmel @ 2021-12-12 11:47 UTC (permalink / raw)
  To: Maciej Machnikowski
  Cc: netdev, intel-wired-lan, arkadiusz.kubalewski, richardcochran,
	abyagowi, anthony.l.nguyen, davem, kuba, linux-kselftest,
	mkubecek, saeed, michael.chan, petrm

On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> Synchronous Ethernet networks use a physical layer clock to syntonize
> the frequency across different network elements.
> 
> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> Equipment Clock (EEC) and have the ability to synchronize to reference
> frequency sources.
> 
> This patch series is a prerequisite for EEC object and adds ability
> to enable recovered clocks in the physical layer of the netdev object.
> Recovered clocks can be used as one of the reference signal by the EEC.

The dependency is the other way around. It doesn't make sense to add
APIs to configure the inputs of an object that doesn't exist. First add
the EEC object, then we can talk about APIs to configure its inputs from
netdevs.

With these four patches alone, user space doesn't know how many EECs
there are in the system, it doesn't know the mapping from netdev to EEC,
it doesn't know the state of the EEC, it doesn't know which source is
chosen in case more than one source is enabled. Patch #3 tries to work
around it by having ice print to kernel log, when the information should
really be exposed via the EEC object.

+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_SYNCE,
+			 pf->synce_dpll_state,
+			 pin);

> 
> Further work is required to add the DPLL subsystem, link it to the
> netdev object and create API to read the EEC DPLL state.

When the EEC object materializes, we might find out that this API needs
to be changed / reworked / removed, but we won't be able to do that
given it's uAPI. I don't know where the confidence that it won't happen
stems from when there are so many question marks around this new
object.

> 
> v5:
> - rewritten the documentation
> - fixed doxygen headers
> 
> v4:
> - Dropped EEC_STATE reporting (TBD: DPLL subsystem)
> - moved recovered clock configuration to ethtool netlink
> 
> v3:
> - remove RTM_GETRCLKRANGE
> - return state of all possible pins in the RTM_GETRCLKSTATE
> - clarify documentation
> 
> v2:
> - improved documentation
> - fixed kdoc warning
> 
> RFC history:
> v2:
> - removed whitespace changes
> - fix issues reported by test robot
> v3:
> - Changed naming from SyncE to EEC
> - Clarify cover letter and commit message for patch 1
> v4:
> - Removed sync_source and pin_idx info
> - Changed one structure to attributes
> - Added EEC_SRC_PORT flag to indicate that the EEC is synchronized
>   to the recovered clock of a port that returns the state
> v5:
> - add EEC source as an optiona attribute
> - implement support for recovered clocks
> - align states returned by EEC to ITU-T G.781
> v6:
> - fix EEC clock state reporting
> - add documentation
> - fix descriptions in code comments
> 
> 
> Maciej Machnikowski (4):
>   ice: add support detecting features based on netlist
>   ethtool: Add ability to configure recovered clock for SyncE feature
>   ice: add support for monitoring SyncE DPLL state
>   ice: add support for recovered clocks
> 
>  Documentation/networking/ethtool-netlink.rst  |  62 ++++
>  drivers/net/ethernet/intel/ice/ice.h          |   7 +
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  70 ++++-
>  drivers/net/ethernet/intel/ice/ice_common.c   | 224 +++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_common.h   |  20 +-
>  drivers/net/ethernet/intel/ice/ice_devids.h   |   3 +
>  drivers/net/ethernet/intel/ice/ice_ethtool.c  |  96 +++++++
>  drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
>  drivers/net/ethernet/intel/ice/ice_ptp.c      |  35 +++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  49 ++++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  36 +++
>  drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>  include/linux/ethtool.h                       |   9 +
>  include/uapi/linux/ethtool_netlink.h          |  21 ++
>  net/ethtool/Makefile                          |   3 +-
>  net/ethtool/netlink.c                         |  20 ++
>  net/ethtool/netlink.h                         |   4 +
>  net/ethtool/synce.c                           | 267 ++++++++++++++++++
>  18 files changed, 929 insertions(+), 4 deletions(-)
>  create mode 100644 net/ethtool/synce.c
> 
> -- 
> 2.26.3
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-12 11:47   ` Ido Schimmel
  0 siblings, 0 replies; 30+ messages in thread
From: Ido Schimmel @ 2021-12-12 11:47 UTC (permalink / raw)
  To: intel-wired-lan

On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> Synchronous Ethernet networks use a physical layer clock to syntonize
> the frequency across different network elements.
> 
> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> Equipment Clock (EEC) and have the ability to synchronize to reference
> frequency sources.
> 
> This patch series is a prerequisite for EEC object and adds ability
> to enable recovered clocks in the physical layer of the netdev object.
> Recovered clocks can be used as one of the reference signal by the EEC.

The dependency is the other way around. It doesn't make sense to add
APIs to configure the inputs of an object that doesn't exist. First add
the EEC object, then we can talk about APIs to configure its inputs from
netdevs.

With these four patches alone, user space doesn't know how many EECs
there are in the system, it doesn't know the mapping from netdev to EEC,
it doesn't know the state of the EEC, it doesn't know which source is
chosen in case more than one source is enabled. Patch #3 tries to work
around it by having ice print to kernel log, when the information should
really be exposed via the EEC object.

+		dev_warn(ice_pf_to_dev(pf),
+			 "<DPLL%i> state changed to: %d, pin %d",
+			 ICE_CGU_DPLL_SYNCE,
+			 pf->synce_dpll_state,
+			 pin);

> 
> Further work is required to add the DPLL subsystem, link it to the
> netdev object and create API to read the EEC DPLL state.

When the EEC object materializes, we might find out that this API needs
to be changed / reworked / removed, but we won't be able to do that
given it's uAPI. I don't know where the confidence that it won't happen
stems from when there are so many question marks around this new
object.

> 
> v5:
> - rewritten the documentation
> - fixed doxygen headers
> 
> v4:
> - Dropped EEC_STATE reporting (TBD: DPLL subsystem)
> - moved recovered clock configuration to ethtool netlink
> 
> v3:
> - remove RTM_GETRCLKRANGE
> - return state of all possible pins in the RTM_GETRCLKSTATE
> - clarify documentation
> 
> v2:
> - improved documentation
> - fixed kdoc warning
> 
> RFC history:
> v2:
> - removed whitespace changes
> - fix issues reported by test robot
> v3:
> - Changed naming from SyncE to EEC
> - Clarify cover letter and commit message for patch 1
> v4:
> - Removed sync_source and pin_idx info
> - Changed one structure to attributes
> - Added EEC_SRC_PORT flag to indicate that the EEC is synchronized
>   to the recovered clock of a port that returns the state
> v5:
> - add EEC source as an optiona attribute
> - implement support for recovered clocks
> - align states returned by EEC to ITU-T G.781
> v6:
> - fix EEC clock state reporting
> - add documentation
> - fix descriptions in code comments
> 
> 
> Maciej Machnikowski (4):
>   ice: add support detecting features based on netlist
>   ethtool: Add ability to configure recovered clock for SyncE feature
>   ice: add support for monitoring SyncE DPLL state
>   ice: add support for recovered clocks
> 
>  Documentation/networking/ethtool-netlink.rst  |  62 ++++
>  drivers/net/ethernet/intel/ice/ice.h          |   7 +
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  70 ++++-
>  drivers/net/ethernet/intel/ice/ice_common.c   | 224 +++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_common.h   |  20 +-
>  drivers/net/ethernet/intel/ice/ice_devids.h   |   3 +
>  drivers/net/ethernet/intel/ice/ice_ethtool.c  |  96 +++++++
>  drivers/net/ethernet/intel/ice/ice_lib.c      |   6 +-
>  drivers/net/ethernet/intel/ice/ice_ptp.c      |  35 +++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  49 ++++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  36 +++
>  drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>  include/linux/ethtool.h                       |   9 +
>  include/uapi/linux/ethtool_netlink.h          |  21 ++
>  net/ethtool/Makefile                          |   3 +-
>  net/ethtool/netlink.c                         |  20 ++
>  net/ethtool/netlink.h                         |   4 +
>  net/ethtool/synce.c                           | 267 ++++++++++++++++++
>  18 files changed, 929 insertions(+), 4 deletions(-)
>  create mode 100644 net/ethtool/synce.c
> 
> -- 
> 2.26.3
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-10 16:16   ` [Intel-wired-lan] " Jakub Kicinski
@ 2021-12-13  8:53     ` Machnikowski, Maciej
  -1 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-13  8:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, intel-wired-lan, Kubalewski, Arkadiusz, richardcochran,
	Byagowi, Ahmad, Nguyen, Anthony L, davem, linux-kselftest,
	idosch, mkubecek, saeed, michael.chan, petrm, Vadim Fedorenko



> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, December 10, 2021 5:17 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran@gmail.com;
> Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-
> kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz;
> saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com;
> Vadim Fedorenko <vfedorenko@novek.ru>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> > Synchronous Ethernet networks use a physical layer clock to syntonize
> > the frequency across different network elements.
> >
> > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > Equipment Clock (EEC) and have the ability to synchronize to reference
> > frequency sources.
> >
> > This patch series is a prerequisite for EEC object and adds ability
> > to enable recovered clocks in the physical layer of the netdev object.
> > Recovered clocks can be used as one of the reference signal by the EEC.
> >
> > Further work is required to add the DPLL subsystem, link it to the
> > netdev object and create API to read the EEC DPLL state.
> 
> You missed CCing Vadim. I guess Ccing the right people may be right up
> there with naming things as the hardest things in SW development..
> 
> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?

Sounds about right :) thanks for adding Vadim!

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-13  8:53     ` Machnikowski, Maciej
  0 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-13  8:53 UTC (permalink / raw)
  To: intel-wired-lan



> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, December 10, 2021 5:17 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran at gmail.com;
> Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-
> kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz;
> saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com;
> Vadim Fedorenko <vfedorenko@novek.ru>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> > Synchronous Ethernet networks use a physical layer clock to syntonize
> > the frequency across different network elements.
> >
> > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > Equipment Clock (EEC) and have the ability to synchronize to reference
> > frequency sources.
> >
> > This patch series is a prerequisite for EEC object and adds ability
> > to enable recovered clocks in the physical layer of the netdev object.
> > Recovered clocks can be used as one of the reference signal by the EEC.
> >
> > Further work is required to add the DPLL subsystem, link it to the
> > netdev object and create API to read the EEC DPLL state.
> 
> You missed CCing Vadim. I guess Ccing the right people may be right up
> there with naming things as the hardest things in SW development..
> 
> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?

Sounds about right :) thanks for adding Vadim!

^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-12 11:47   ` [Intel-wired-lan] " Ido Schimmel
@ 2021-12-15 12:13     ` Machnikowski, Maciej
  -1 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-15 12:13 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: netdev, intel-wired-lan, Kubalewski, Arkadiusz, richardcochran,
	Byagowi, Ahmad, Nguyen, Anthony L, davem, kuba, linux-kselftest,
	mkubecek, saeed, michael.chan, petrm, Vadim Fedorenko

> -----Original Message-----
> From: Ido Schimmel <idosch@idosch.org>
> Sent: Sunday, December 12, 2021 12:48 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > Synchronous Ethernet networks use a physical layer clock to syntonize
> > the frequency across different network elements.
> >
> > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > Equipment Clock (EEC) and have the ability to synchronize to reference
> > frequency sources.
> >
> > This patch series is a prerequisite for EEC object and adds ability
> > to enable recovered clocks in the physical layer of the netdev object.
> > Recovered clocks can be used as one of the reference signal by the EEC.
> 
> The dependency is the other way around. It doesn't make sense to add
> APIs to configure the inputs of an object that doesn't exist. First add
> the EEC object, then we can talk about APIs to configure its inputs from
> netdevs.

This API configures frequency outputs of the PTY layer of
a PHY/integrated MAC. It does not configure any inputs nor it interacts
with the EEC. The goal of it is to expose the clock to the piece that
requires it as a reference one (a DPLL/FPGA/anything else).

I don't agree with the statement that we must have EEC object first,
as we can already configure different frequency sources using different
subsystems. The source of signal should be separated from its
consumer.
 
> With these four patches alone, user space doesn't know how many EECs
> there are in the system, it doesn't know the mapping from netdev to EEC,
> it doesn't know the state of the EEC, it doesn't know which source is
> chosen in case more than one source is enabled. Patch #3 tries to work
> around it by having ice print to kernel log, when the information should
> really be exposed via the EEC object.

The goal of them is to add API for recovered clocks - not for EECs. This part 
is there for observability and will still be there when EEC is in place.
Those will need to be addressed by the DPLL subsystem.

> +		dev_warn(ice_pf_to_dev(pf),
> +			 "<DPLL%i> state changed to: %d, pin %d",
> +			 ICE_CGU_DPLL_SYNCE,
> +			 pf->synce_dpll_state,
> +			 pin);
> 
> >
> > Further work is required to add the DPLL subsystem, link it to the
> > netdev object and create API to read the EEC DPLL state.
> 
> When the EEC object materializes, we might find out that this API needs
> to be changed / reworked / removed, but we won't be able to do that
> given it's uAPI. I don't know where the confidence that it won't happen
> stems from when there are so many question marks around this new
> object.

This API follows the functionality of other frequency outputs that exist
in the kernel, like PTP period file for frequency output of PTP clock
or other GPIOs. I highly doubt it'll change - we may extend it to add mapping
between pins, but like I indicated - this will not always be known to SW.

Regards
Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-15 12:13     ` Machnikowski, Maciej
  0 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-15 12:13 UTC (permalink / raw)
  To: intel-wired-lan

> -----Original Message-----
> From: Ido Schimmel <idosch@idosch.org>
> Sent: Sunday, December 12, 2021 12:48 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > Synchronous Ethernet networks use a physical layer clock to syntonize
> > the frequency across different network elements.
> >
> > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > Equipment Clock (EEC) and have the ability to synchronize to reference
> > frequency sources.
> >
> > This patch series is a prerequisite for EEC object and adds ability
> > to enable recovered clocks in the physical layer of the netdev object.
> > Recovered clocks can be used as one of the reference signal by the EEC.
> 
> The dependency is the other way around. It doesn't make sense to add
> APIs to configure the inputs of an object that doesn't exist. First add
> the EEC object, then we can talk about APIs to configure its inputs from
> netdevs.

This API configures frequency outputs of the PTY layer of
a PHY/integrated MAC. It does not configure any inputs nor it interacts
with the EEC. The goal of it is to expose the clock to the piece that
requires it as a reference one (a DPLL/FPGA/anything else).

I don't agree with the statement that we must have EEC object first,
as we can already configure different frequency sources using different
subsystems. The source of signal should be separated from its
consumer.
 
> With these four patches alone, user space doesn't know how many EECs
> there are in the system, it doesn't know the mapping from netdev to EEC,
> it doesn't know the state of the EEC, it doesn't know which source is
> chosen in case more than one source is enabled. Patch #3 tries to work
> around it by having ice print to kernel log, when the information should
> really be exposed via the EEC object.

The goal of them is to add API for recovered clocks - not for EECs. This part 
is there for observability and will still be there when EEC is in place.
Those will need to be addressed by the DPLL subsystem.

> +		dev_warn(ice_pf_to_dev(pf),
> +			 "<DPLL%i> state changed to: %d, pin %d",
> +			 ICE_CGU_DPLL_SYNCE,
> +			 pf->synce_dpll_state,
> +			 pin);
> 
> >
> > Further work is required to add the DPLL subsystem, link it to the
> > netdev object and create API to read the EEC DPLL state.
> 
> When the EEC object materializes, we might find out that this API needs
> to be changed / reworked / removed, but we won't be able to do that
> given it's uAPI. I don't know where the confidence that it won't happen
> stems from when there are so many question marks around this new
> object.

This API follows the functionality of other frequency outputs that exist
in the kernel, like PTP period file for frequency output of PTP clock
or other GPIOs. I highly doubt it'll change - we may extend it to add mapping
between pins, but like I indicated - this will not always be known to SW.

Regards
Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-13  8:53     ` [Intel-wired-lan] " Machnikowski, Maciej
@ 2021-12-15 12:14       ` Kubalewski, Arkadiusz
  -1 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2021-12-15 12:14 UTC (permalink / raw)
  To: Machnikowski, Maciej, Jakub Kicinski, Vadim Fedorenko
  Cc: netdev, intel-wired-lan, richardcochran, Byagowi, Ahmad, Nguyen,
	Anthony L, davem, linux-kselftest, idosch, mkubecek, saeed,
	michael.chan, petrm

> -----Original Message-----
> From: Machnikowski, Maciej <maciej.machnikowski@intel.com> 
> Sent: poniedziałek, 13 grudnia 2021 09:54
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz; saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, December 10, 2021 5:17 PM
> > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; 
> > Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
> > richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
> > Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux- 
> > kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz; 
> > saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim 
> > Fedorenko <vfedorenko@novek.ru>
> > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > 
> > On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> > > Synchronous Ethernet networks use a physical layer clock to 
> > > syntonize the frequency across different network elements.
> > >
> > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet 
> > > Equipment Clock (EEC) and have the ability to synchronize to 
> > > reference frequency sources.
> > >
> > > This patch series is a prerequisite for EEC object and adds ability 
> > > to enable recovered clocks in the physical layer of the netdev object.
> > > Recovered clocks can be used as one of the reference signal by the EEC.
> > >
> > > Further work is required to add the DPLL subsystem, link it to the 
> > > netdev object and create API to read the EEC DPLL state.
> > 
> > You missed CCing Vadim. I guess Ccing the right people may be right up 
> > there with naming things as the hardest things in SW development..
> > 
> > Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
> 
> Sounds about right :) thanks for adding Vadim!
> 

Good day Vadim,

Can we help on the new PLL interfaces?
I can start some works related to that, although would need a guidance
from the expert. 
Where to place it?
What in-kernel interfaces to use?
Any other high level tips that could be useful?
Or if you already started some work, could you please share some
information?

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-15 12:14       ` Kubalewski, Arkadiusz
  0 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2021-12-15 12:14 UTC (permalink / raw)
  To: intel-wired-lan

> -----Original Message-----
> From: Machnikowski, Maciej <maciej.machnikowski@intel.com> 
> Sent: poniedzia?ek, 13 grudnia 2021 09:54
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz; saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, December 10, 2021 5:17 PM
> > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; 
> > Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
> > richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
> > Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux- 
> > kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz; 
> > saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim 
> > Fedorenko <vfedorenko@novek.ru>
> > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > 
> > On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
> > > Synchronous Ethernet networks use a physical layer clock to 
> > > syntonize the frequency across different network elements.
> > >
> > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet 
> > > Equipment Clock (EEC) and have the ability to synchronize to 
> > > reference frequency sources.
> > >
> > > This patch series is a prerequisite for EEC object and adds ability 
> > > to enable recovered clocks in the physical layer of the netdev object.
> > > Recovered clocks can be used as one of the reference signal by the EEC.
> > >
> > > Further work is required to add the DPLL subsystem, link it to the 
> > > netdev object and create API to read the EEC DPLL state.
> > 
> > You missed CCing Vadim. I guess Ccing the right people may be right up 
> > there with naming things as the hardest things in SW development..
> > 
> > Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
> 
> Sounds about right :) thanks for adding Vadim!
> 

Good day Vadim,

Can we help on the new PLL interfaces?
I can start some works related to that, although would need a guidance
from the expert. 
Where to place it?
What in-kernel interfaces to use?
Any other high level tips that could be useful?
Or if you already started some work, could you please share some
information?

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-15 12:14       ` [Intel-wired-lan] " Kubalewski, Arkadiusz
@ 2021-12-15 22:27         ` Vadim Fedorenko
  -1 siblings, 0 replies; 30+ messages in thread
From: Vadim Fedorenko @ 2021-12-15 22:27 UTC (permalink / raw)
  To: Kubalewski, Arkadiusz, Machnikowski, Maciej, Jakub Kicinski
  Cc: netdev, intel-wired-lan, richardcochran, Byagowi, Ahmad, Nguyen,
	Anthony L, davem, linux-kselftest, idosch, mkubecek, saeed,
	michael.chan, petrm

On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>> Sent: poniedziałek, 13 grudnia 2021 09:54
>> To: Jakub Kicinski <kuba@kernel.org>
>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz; saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>
>>> -----Original Message-----
>>> From: Jakub Kicinski <kuba@kernel.org>
>>> Sent: Friday, December 10, 2021 5:17 PM
>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>;
>>> richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen,
>>> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-
>>> kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz;
>>> saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim
>>> Fedorenko <vfedorenko@novek.ru>
>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>
>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>> Synchronous Ethernet networks use a physical layer clock to
>>>> syntonize the frequency across different network elements.
>>>>
>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
>>>> Equipment Clock (EEC) and have the ability to synchronize to
>>>> reference frequency sources.
>>>>
>>>> This patch series is a prerequisite for EEC object and adds ability
>>>> to enable recovered clocks in the physical layer of the netdev object.
>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>
>>>> Further work is required to add the DPLL subsystem, link it to the
>>>> netdev object and create API to read the EEC DPLL state.
>>>
>>> You missed CCing Vadim. I guess Ccing the right people may be right up
>>> there with naming things as the hardest things in SW development..
>>>
>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>
>> Sounds about right :) thanks for adding Vadim!
>>
> 
> Good day Vadim,
> 
> Can we help on the new PLL interfaces?
> I can start some works related to that, although would need a guidance
> from the expert.
> Where to place it?
> What in-kernel interfaces to use?
> Any other high level tips that could be useful?
> Or if you already started some work, could you please share some
> information?
> 
Hi!

I'm going to publish RFC till the end of the week and we will be able to
continue discussion via this mailing list. I think that netlink is a good
option for in-kernel interface and is easy to implement.


^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-15 22:27         ` Vadim Fedorenko
  0 siblings, 0 replies; 30+ messages in thread
From: Vadim Fedorenko @ 2021-12-15 22:27 UTC (permalink / raw)
  To: intel-wired-lan

On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>> Sent: poniedzia?ek, 13 grudnia 2021 09:54
>> To: Jakub Kicinski <kuba@kernel.org>
>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz; saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>
>>> -----Original Message-----
>>> From: Jakub Kicinski <kuba@kernel.org>
>>> Sent: Friday, December 10, 2021 5:17 PM
>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org;
>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>;
>>> richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen,
>>> Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-
>>> kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz;
>>> saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim
>>> Fedorenko <vfedorenko@novek.ru>
>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>
>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>> Synchronous Ethernet networks use a physical layer clock to
>>>> syntonize the frequency across different network elements.
>>>>
>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
>>>> Equipment Clock (EEC) and have the ability to synchronize to
>>>> reference frequency sources.
>>>>
>>>> This patch series is a prerequisite for EEC object and adds ability
>>>> to enable recovered clocks in the physical layer of the netdev object.
>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>
>>>> Further work is required to add the DPLL subsystem, link it to the
>>>> netdev object and create API to read the EEC DPLL state.
>>>
>>> You missed CCing Vadim. I guess Ccing the right people may be right up
>>> there with naming things as the hardest things in SW development..
>>>
>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>
>> Sounds about right :) thanks for adding Vadim!
>>
> 
> Good day Vadim,
> 
> Can we help on the new PLL interfaces?
> I can start some works related to that, although would need a guidance
> from the expert.
> Where to place it?
> What in-kernel interfaces to use?
> Any other high level tips that could be useful?
> Or if you already started some work, could you please share some
> information?
> 
Hi!

I'm going to publish RFC till the end of the week and we will be able to
continue discussion via this mailing list. I think that netlink is a good
option for in-kernel interface and is easy to implement.


^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-15 22:27         ` [Intel-wired-lan] " Vadim Fedorenko
@ 2021-12-17  0:01           ` Kubalewski, Arkadiusz
  -1 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2021-12-17  0:01 UTC (permalink / raw)
  To: Vadim Fedorenko, Machnikowski, Maciej, Jakub Kicinski
  Cc: netdev, intel-wired-lan, richardcochran, Byagowi, Ahmad, Nguyen,
	Anthony L, davem, linux-kselftest, idosch, mkubecek, saeed,
	michael.chan, petrm

>On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>>> -----Original Message-----
>>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>> Sent: poniedziałek, 13 grudnia 2021 09:54
>>> To: Jakub Kicinski <kuba@kernel.org>
>>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz; saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>
>>>> -----Original Message-----
>>>> From: Jakub Kicinski <kuba@kernel.org>
>>>> Sent: Friday, December 10, 2021 5:17 PM
>>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>;
>>>> richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen,
>>>> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux-
>>>> kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz;
>>>> saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; Vadim
>>>> Fedorenko <vfedorenko@novek.ru>
>>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>>
>>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>>> Synchronous Ethernet networks use a physical layer clock to
>>>>> syntonize the frequency across different network elements.
>>>>>
>>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
>>>>> Equipment Clock (EEC) and have the ability to synchronize to
>>>>> reference frequency sources.
>>>>>
>>>>> This patch series is a prerequisite for EEC object and adds ability
>>>>> to enable recovered clocks in the physical layer of the netdev object.
>>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>>
>>>>> Further work is required to add the DPLL subsystem, link it to the
>>>>> netdev object and create API to read the EEC DPLL state.
>>>>
>>>> You missed CCing Vadim. I guess Ccing the right people may be right up
>>>> there with naming things as the hardest things in SW development..
>>>>
>>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>>
>>> Sounds about right :) thanks for adding Vadim!
>>>
>> 
>> Good day Vadim,
>> 
>> Can we help on the new PLL interfaces?
>> I can start some works related to that, although would need a guidance
>> from the expert.
>> Where to place it?
>> What in-kernel interfaces to use?
>> Any other high level tips that could be useful?
>> Or if you already started some work, could you please share some
>> information?
>> 
>Hi!
>
>I'm going to publish RFC till the end of the week and we will be able to
>continue discussion via this mailing list. I think that netlink is a good
>option for in-kernel interface and is easy to implement.
>

Oh, that sounds great!
Thank you!

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-17  0:01           ` Kubalewski, Arkadiusz
  0 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2021-12-17  0:01 UTC (permalink / raw)
  To: intel-wired-lan

>On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>>> -----Original Message-----
>>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>> Sent: poniedzia?ek, 13 grudnia 2021 09:54
>>> To: Jakub Kicinski <kuba@kernel.org>
>>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz; saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>
>>>> -----Original Message-----
>>>> From: Jakub Kicinski <kuba@kernel.org>
>>>> Sent: Friday, December 10, 2021 5:17 PM
>>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org;
>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>;
>>>> richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen,
>>>> Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux-
>>>> kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz;
>>>> saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; Vadim
>>>> Fedorenko <vfedorenko@novek.ru>
>>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
>>>>
>>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>>> Synchronous Ethernet networks use a physical layer clock to
>>>>> syntonize the frequency across different network elements.
>>>>>
>>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
>>>>> Equipment Clock (EEC) and have the ability to synchronize to
>>>>> reference frequency sources.
>>>>>
>>>>> This patch series is a prerequisite for EEC object and adds ability
>>>>> to enable recovered clocks in the physical layer of the netdev object.
>>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>>
>>>>> Further work is required to add the DPLL subsystem, link it to the
>>>>> netdev object and create API to read the EEC DPLL state.
>>>>
>>>> You missed CCing Vadim. I guess Ccing the right people may be right up
>>>> there with naming things as the hardest things in SW development..
>>>>
>>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>>
>>> Sounds about right :) thanks for adding Vadim!
>>>
>> 
>> Good day Vadim,
>> 
>> Can we help on the new PLL interfaces?
>> I can start some works related to that, although would need a guidance
>> from the expert.
>> Where to place it?
>> What in-kernel interfaces to use?
>> Any other high level tips that could be useful?
>> Or if you already started some work, could you please share some
>> information?
>> 
>Hi!
>
>I'm going to publish RFC till the end of the week and we will be able to
>continue discussion via this mailing list. I think that netlink is a good
>option for in-kernel interface and is easy to implement.
>

Oh, that sounds great!
Thank you!

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-15 12:13     ` [Intel-wired-lan] " Machnikowski, Maciej
@ 2021-12-20 13:50       ` Ido Schimmel
  -1 siblings, 0 replies; 30+ messages in thread
From: Ido Schimmel @ 2021-12-20 13:50 UTC (permalink / raw)
  To: Machnikowski, Maciej
  Cc: netdev, intel-wired-lan, Kubalewski, Arkadiusz, richardcochran,
	Byagowi, Ahmad, Nguyen, Anthony L, davem, kuba, linux-kselftest,
	mkubecek, saeed, michael.chan, petrm, Vadim Fedorenko

On Wed, Dec 15, 2021 at 12:13:47PM +0000, Machnikowski, Maciej wrote:
> > -----Original Message-----
> > From: Ido Schimmel <idosch@idosch.org>
> > Sent: Sunday, December 12, 2021 12:48 PM
> > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > 
> > On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > > Synchronous Ethernet networks use a physical layer clock to syntonize
> > > the frequency across different network elements.
> > >
> > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > > Equipment Clock (EEC) and have the ability to synchronize to reference
> > > frequency sources.
> > >
> > > This patch series is a prerequisite for EEC object and adds ability
> > > to enable recovered clocks in the physical layer of the netdev object.
> > > Recovered clocks can be used as one of the reference signal by the EEC.
> > 
> > The dependency is the other way around. It doesn't make sense to add
> > APIs to configure the inputs of an object that doesn't exist. First add
> > the EEC object, then we can talk about APIs to configure its inputs from
> > netdevs.
> 
> This API configures frequency outputs of the PTY layer of
> a PHY/integrated MAC. It does not configure any inputs nor it interacts
> with the EEC. The goal of it is to expose the clock to the piece that
> requires it as a reference one (a DPLL/FPGA/anything else).

My fundamental issue with these patches is that instead of abstracting
the hardware from user space they give user space direct control over
it.

This approach has the advantage of keeping the kernel relatively simple
and fitting more use cases than just EEC, but these are also its
disadvantages. Complexity needs to live somewhere and if this complexity
is related to the abstraction of hardware, then it should live in the
kernel and not in user space. We should strive to come up with an API
that does the same thing regardless of the underlying hardware
implementation.

Look at the proposed API, it basically says "Make the clock recovered
from eth0 available on pin 1". If user space issues this command on
different systems, it will mean different things, based on the
underlying design of the hardware and the connection of the pin: To
"DPLL/FPGA/anything else".

Contrast that with an API that says "Set the source of EEC X to the
clock recovered from eth0". This API is well defined and does the same
thing across different systems.

Lets assume that these patches are merged as-is and tomorrow we get
another implementation of these two ethtool operations in a different
driver. We can't tell if these are used to feed the recovered clock into
an EEC like ice does or enable some other use case that we never
intended to enable.

Even if all the implementations use this API to feed the EEC, consider
how difficult it is going to be for user space to use it. Ideally, user
space should be able to query the state of the EEC and its source via an
EEC object in a single command. With the current approach in which we
have some amorphic object called "DPLL" that is only aware of pins and
not netdevs, it's going to be very hard. User space will see that the
DPLL is locked to the clock fed via pin 1. How user space is supposed to
understand what is the source of this clock? Issue RCLK_GET dump and
check for matching pin index that is enabled? User space has no reason
to do it given that it doesn't even know that the source is a netdev.

> 
> I don't agree with the statement that we must have EEC object first,
> as we can already configure different frequency sources using different
> subsystems.

Regardless of all the above technical arguments, I think that these
patches should not be merged now based on common sense alone. Not only
these patches are of very limited use without an EEC object, they also
prevent us from making changes to the API when such an object is
introduced.

> The source of signal should be separated from its consumer.

If it is completely separated (despite being hardwired on the board),
then user space does not know how the signal is used when it issues the
command. Is this signal fed into an EEC that controls that transmission
rate of other netdev? Is this signal fed into an FPGA that blinks a led?

>  
> > With these four patches alone, user space doesn't know how many EECs
> > there are in the system, it doesn't know the mapping from netdev to EEC,
> > it doesn't know the state of the EEC, it doesn't know which source is
> > chosen in case more than one source is enabled. Patch #3 tries to work
> > around it by having ice print to kernel log, when the information should
> > really be exposed via the EEC object.
> 
> The goal of them is to add API for recovered clocks - not for EECs.

What do you mean by "not for EECs"? The file is called
"net/ethtool/synce.c", if the signal is not being fed into an EEC then
into what? It is unclear what kind of back doors this API will open.

> This part is there for observability and will still be there when EEC
> is in place.  Those will need to be addressed by the DPLL subsystem.

If it is it only meant for observability, then why these messages are
emitted as warnings to the kernel log? Regardless, the user API should
be designed with observability in mind so that you wouldn't need to rely
on prints to the kernel log.

> 
> > +		dev_warn(ice_pf_to_dev(pf),
> > +			 "<DPLL%i> state changed to: %d, pin %d",
> > +			 ICE_CGU_DPLL_SYNCE,
> > +			 pf->synce_dpll_state,
> > +			 pin);
> > 
> > >
> > > Further work is required to add the DPLL subsystem, link it to the
> > > netdev object and create API to read the EEC DPLL state.
> > 
> > When the EEC object materializes, we might find out that this API needs
> > to be changed / reworked / removed, but we won't be able to do that
> > given it's uAPI. I don't know where the confidence that it won't happen
> > stems from when there are so many question marks around this new
> > object.
> 
> This API follows the functionality of other frequency outputs that exist
> in the kernel, like PTP period file for frequency output of PTP clock
> or other GPIOs. I highly doubt it'll change - we may extend it to add mapping
> between pins, but like I indicated - this will not always be known to SW.
> 
> Regards
> Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-20 13:50       ` Ido Schimmel
  0 siblings, 0 replies; 30+ messages in thread
From: Ido Schimmel @ 2021-12-20 13:50 UTC (permalink / raw)
  To: intel-wired-lan

On Wed, Dec 15, 2021 at 12:13:47PM +0000, Machnikowski, Maciej wrote:
> > -----Original Message-----
> > From: Ido Schimmel <idosch@idosch.org>
> > Sent: Sunday, December 12, 2021 12:48 PM
> > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > 
> > On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > > Synchronous Ethernet networks use a physical layer clock to syntonize
> > > the frequency across different network elements.
> > >
> > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > > Equipment Clock (EEC) and have the ability to synchronize to reference
> > > frequency sources.
> > >
> > > This patch series is a prerequisite for EEC object and adds ability
> > > to enable recovered clocks in the physical layer of the netdev object.
> > > Recovered clocks can be used as one of the reference signal by the EEC.
> > 
> > The dependency is the other way around. It doesn't make sense to add
> > APIs to configure the inputs of an object that doesn't exist. First add
> > the EEC object, then we can talk about APIs to configure its inputs from
> > netdevs.
> 
> This API configures frequency outputs of the PTY layer of
> a PHY/integrated MAC. It does not configure any inputs nor it interacts
> with the EEC. The goal of it is to expose the clock to the piece that
> requires it as a reference one (a DPLL/FPGA/anything else).

My fundamental issue with these patches is that instead of abstracting
the hardware from user space they give user space direct control over
it.

This approach has the advantage of keeping the kernel relatively simple
and fitting more use cases than just EEC, but these are also its
disadvantages. Complexity needs to live somewhere and if this complexity
is related to the abstraction of hardware, then it should live in the
kernel and not in user space. We should strive to come up with an API
that does the same thing regardless of the underlying hardware
implementation.

Look at the proposed API, it basically says "Make the clock recovered
from eth0 available on pin 1". If user space issues this command on
different systems, it will mean different things, based on the
underlying design of the hardware and the connection of the pin: To
"DPLL/FPGA/anything else".

Contrast that with an API that says "Set the source of EEC X to the
clock recovered from eth0". This API is well defined and does the same
thing across different systems.

Lets assume that these patches are merged as-is and tomorrow we get
another implementation of these two ethtool operations in a different
driver. We can't tell if these are used to feed the recovered clock into
an EEC like ice does or enable some other use case that we never
intended to enable.

Even if all the implementations use this API to feed the EEC, consider
how difficult it is going to be for user space to use it. Ideally, user
space should be able to query the state of the EEC and its source via an
EEC object in a single command. With the current approach in which we
have some amorphic object called "DPLL" that is only aware of pins and
not netdevs, it's going to be very hard. User space will see that the
DPLL is locked to the clock fed via pin 1. How user space is supposed to
understand what is the source of this clock? Issue RCLK_GET dump and
check for matching pin index that is enabled? User space has no reason
to do it given that it doesn't even know that the source is a netdev.

> 
> I don't agree with the statement that we must have EEC object first,
> as we can already configure different frequency sources using different
> subsystems.

Regardless of all the above technical arguments, I think that these
patches should not be merged now based on common sense alone. Not only
these patches are of very limited use without an EEC object, they also
prevent us from making changes to the API when such an object is
introduced.

> The source of signal should be separated from its consumer.

If it is completely separated (despite being hardwired on the board),
then user space does not know how the signal is used when it issues the
command. Is this signal fed into an EEC that controls that transmission
rate of other netdev? Is this signal fed into an FPGA that blinks a led?

>  
> > With these four patches alone, user space doesn't know how many EECs
> > there are in the system, it doesn't know the mapping from netdev to EEC,
> > it doesn't know the state of the EEC, it doesn't know which source is
> > chosen in case more than one source is enabled. Patch #3 tries to work
> > around it by having ice print to kernel log, when the information should
> > really be exposed via the EEC object.
> 
> The goal of them is to add API for recovered clocks - not for EECs.

What do you mean by "not for EECs"? The file is called
"net/ethtool/synce.c", if the signal is not being fed into an EEC then
into what? It is unclear what kind of back doors this API will open.

> This part is there for observability and will still be there when EEC
> is in place.  Those will need to be addressed by the DPLL subsystem.

If it is it only meant for observability, then why these messages are
emitted as warnings to the kernel log? Regardless, the user API should
be designed with observability in mind so that you wouldn't need to rely
on prints to the kernel log.

> 
> > +		dev_warn(ice_pf_to_dev(pf),
> > +			 "<DPLL%i> state changed to: %d, pin %d",
> > +			 ICE_CGU_DPLL_SYNCE,
> > +			 pf->synce_dpll_state,
> > +			 pin);
> > 
> > >
> > > Further work is required to add the DPLL subsystem, link it to the
> > > netdev object and create API to read the EEC DPLL state.
> > 
> > When the EEC object materializes, we might find out that this API needs
> > to be changed / reworked / removed, but we won't be able to do that
> > given it's uAPI. I don't know where the confidence that it won't happen
> > stems from when there are so many question marks around this new
> > object.
> 
> This API follows the functionality of other frequency outputs that exist
> in the kernel, like PTP period file for frequency output of PTP clock
> or other GPIOs. I highly doubt it'll change - we may extend it to add mapping
> between pins, but like I indicated - this will not always be known to SW.
> 
> Regards
> Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-20 13:50       ` [Intel-wired-lan] " Ido Schimmel
@ 2021-12-21 22:12         ` Machnikowski, Maciej
  -1 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-21 22:12 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: netdev, intel-wired-lan, Kubalewski, Arkadiusz, richardcochran,
	Byagowi, Ahmad, Nguyen, Anthony L, davem, kuba, linux-kselftest,
	mkubecek, saeed, michael.chan, petrm, Vadim Fedorenko

> -----Original Message-----
> From: Ido Schimmel <idosch@idosch.org>
> Sent: Monday, December 20, 2021 2:51 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Wed, Dec 15, 2021 at 12:13:47PM +0000, Machnikowski, Maciej wrote:
> > > -----Original Message-----
> > > From: Ido Schimmel <idosch@idosch.org>
> > > Sent: Sunday, December 12, 2021 12:48 PM
> > > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > >
> > > On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > > > Synchronous Ethernet networks use a physical layer clock to syntonize
> > > > the frequency across different network elements.
> > > >
> > > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > > > Equipment Clock (EEC) and have the ability to synchronize to reference
> > > > frequency sources.
> > > >
> > > > This patch series is a prerequisite for EEC object and adds ability
> > > > to enable recovered clocks in the physical layer of the netdev object.
> > > > Recovered clocks can be used as one of the reference signal by the EEC.
> > >
> > > The dependency is the other way around. It doesn't make sense to add
> > > APIs to configure the inputs of an object that doesn't exist. First add
> > > the EEC object, then we can talk about APIs to configure its inputs from
> > > netdevs.
> >
> > This API configures frequency outputs of the PTY layer of
> > a PHY/integrated MAC. It does not configure any inputs nor it interacts
> > with the EEC. The goal of it is to expose the clock to the piece that
> > requires it as a reference one (a DPLL/FPGA/anything else).
> 
> My fundamental issue with these patches is that instead of abstracting
> the hardware from user space they give user space direct control over
> it.

Unfortunately, I don't see any other way to address the use cases that I listed
otherwise. I'm a big fan of simple solutions, but only when they address
all use cases that are known at the time of defining them.

> This approach has the advantage of keeping the kernel relatively simple
> and fitting more use cases than just EEC, but these are also its
> disadvantages. Complexity needs to live somewhere and if this complexity
> is related to the abstraction of hardware, then it should live in the
> kernel and not in user space. We should strive to come up with an API
> that does the same thing regardless of the underlying hardware
> implementation.
> 
> Look at the proposed API, it basically says "Make the clock recovered
> from eth0 available on pin 1". If user space issues this command on
> different systems, it will mean different things, based on the
> underlying design of the hardware and the connection of the pin: To
> "DPLL/FPGA/anything else".

The link to the right DPLL input can be easily added later by adding
the new attribute to those commands. This has a big advantage
over the other model, as it is scalable to multiple different devices
serviced by different drivers, as it doesn't have the issue we have with
linking objects in kernel...

> Contrast that with an API that says "Set the source of EEC X to the
> clock recovered from eth0". This API is well defined and does the same
> thing across different systems.
> 
> Lets assume that these patches are merged as-is and tomorrow we get
> another implementation of these two ethtool operations in a different
> driver. We can't tell if these are used to feed the recovered clock into
> an EEC like ice does or enable some other use case that we never
> intended to enable.

...that issue is caused by the randomness and scalability of that 
infrastructure. At the time system boots it enumerates devices and
initializes them in  "random" order depending on the current system 
configuration which makes it impossible to code any co-driver 
dependencies, as it's impossible to guess what identifier the kernel
assigned to the other device that's connected to the same DPLL.

> Even if all the implementations use this API to feed the EEC, consider
> how difficult it is going to be for user space to use it. Ideally, user
> space should be able to query the state of the EEC and its source via an
> EEC object in a single command. With the current approach in which we
> have some amorphic object called "DPLL" that is only aware of pins and
> not netdevs, it's going to be very hard. User space will see that the
> DPLL is locked to the clock fed via pin 1. How user space is supposed to
> understand what is the source of this clock? Issue RCLK_GET dump and
> check for matching pin index that is enabled? User space has no reason
> to do it given that it doesn't even know that the source is a netdev.

Userspace tool will always know what ports it works with - it will come
from the config file, as - even if you have a dozen ports linked to a single
DPLL you may decide that you don't want to send ESMC messages
on some of them - just like we do with ptp4l.

The amorphic DPLL device has much more uses that a dedicated EEC
object and if we know what pin the DPLL uses and what is the link
between the RCLK output and the DPLL input it'll be very easy to
figure out what's going on.

The dump will also not be needed, as we know who we set as the best
source after receiving the ESMC packets from neighbors.

> >
> > I don't agree with the statement that we must have EEC object first,
> > as we can already configure different frequency sources using different
> > subsystems.
> 
> Regardless of all the above technical arguments, I think that these
> patches should not be merged now based on common sense alone. Not only
> these patches are of very limited use without an EEC object, they also
> prevent us from making changes to the API when such an object is
> introduced.

And I see a big value of merging them in as that would enable upstreaming
of the userspace tools that will explain a lot and close many gaps.

> > The source of signal should be separated from its consumer.
> 
> If it is completely separated (despite being hardwired on the board),
> then user space does not know how the signal is used when it issues the
> command. Is this signal fed into an EEC that controls that transmission
> rate of other netdev? Is this signal fed into an FPGA that blinks a led?

It's the same thing with frequency pins of the PTP subsystem. Userspace
tool can enable them, but has no idea what's on the other end of them.

> >
> > > With these four patches alone, user space doesn't know how many EECs
> > > there are in the system, it doesn't know the mapping from netdev to EEC,
> > > it doesn't know the state of the EEC, it doesn't know which source is
> > > chosen in case more than one source is enabled. Patch #3 tries to work
> > > around it by having ice print to kernel log, when the information should
> > > really be exposed via the EEC object.
> >
> > The goal of them is to add API for recovered clocks - not for EECs.
> 
> What do you mean by "not for EECs"? The file is called
> "net/ethtool/synce.c", if the signal is not being fed into an EEC then
> into what? It is unclear what kind of back doors this API will open.

Can be a DPLL of the radio part of the device, or different devices that
expects frequency reference.
If it makes it better we can move that to net/ethtool/rclk.c.

> > This part is there for observability and will still be there when EEC
> > is in place.  Those will need to be addressed by the DPLL subsystem.
> 
> If it is it only meant for observability, then why these messages are
> emitted as warnings to the kernel log? Regardless, the user API should
> be designed with observability in mind so that you wouldn't need to rely
> on prints to the kernel log.

You're right - need to change that to dev_info.
And yes - in the final form userspace tools will pull EEC's info from the DPLL
subsystem - this prints the state of 2 DPLLs and is designed for debuggability.

> >
> > > +		dev_warn(ice_pf_to_dev(pf),
> > > +			 "<DPLL%i> state changed to: %d, pin %d",
> > > +			 ICE_CGU_DPLL_SYNCE,
> > > +			 pf->synce_dpll_state,
> > > +			 pin);
> > >
> > > >
> > > > Further work is required to add the DPLL subsystem, link it to the
> > > > netdev object and create API to read the EEC DPLL state.
> > >
> > > When the EEC object materializes, we might find out that this API needs
> > > to be changed / reworked / removed, but we won't be able to do that
> > > given it's uAPI. I don't know where the confidence that it won't happen
> > > stems from when there are so many question marks around this new
> > > object.
> >
> > This API follows the functionality of other frequency outputs that exist
> > in the kernel, like PTP period file for frequency output of PTP clock
> > or other GPIOs. I highly doubt it'll change - we may extend it to add
> mapping
> > between pins, but like I indicated - this will not always be known to SW.
> >
> > Regards
> > Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2021-12-21 22:12         ` Machnikowski, Maciej
  0 siblings, 0 replies; 30+ messages in thread
From: Machnikowski, Maciej @ 2021-12-21 22:12 UTC (permalink / raw)
  To: intel-wired-lan

> -----Original Message-----
> From: Ido Schimmel <idosch@idosch.org>
> Sent: Monday, December 20, 2021 2:51 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Wed, Dec 15, 2021 at 12:13:47PM +0000, Machnikowski, Maciej wrote:
> > > -----Original Message-----
> > > From: Ido Schimmel <idosch@idosch.org>
> > > Sent: Sunday, December 12, 2021 12:48 PM
> > > To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> > > Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> > >
> > > On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > > > Synchronous Ethernet networks use a physical layer clock to syntonize
> > > > the frequency across different network elements.
> > > >
> > > > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > > > Equipment Clock (EEC) and have the ability to synchronize to reference
> > > > frequency sources.
> > > >
> > > > This patch series is a prerequisite for EEC object and adds ability
> > > > to enable recovered clocks in the physical layer of the netdev object.
> > > > Recovered clocks can be used as one of the reference signal by the EEC.
> > >
> > > The dependency is the other way around. It doesn't make sense to add
> > > APIs to configure the inputs of an object that doesn't exist. First add
> > > the EEC object, then we can talk about APIs to configure its inputs from
> > > netdevs.
> >
> > This API configures frequency outputs of the PTY layer of
> > a PHY/integrated MAC. It does not configure any inputs nor it interacts
> > with the EEC. The goal of it is to expose the clock to the piece that
> > requires it as a reference one (a DPLL/FPGA/anything else).
> 
> My fundamental issue with these patches is that instead of abstracting
> the hardware from user space they give user space direct control over
> it.

Unfortunately, I don't see any other way to address the use cases that I listed
otherwise. I'm a big fan of simple solutions, but only when they address
all use cases that are known at the time of defining them.

> This approach has the advantage of keeping the kernel relatively simple
> and fitting more use cases than just EEC, but these are also its
> disadvantages. Complexity needs to live somewhere and if this complexity
> is related to the abstraction of hardware, then it should live in the
> kernel and not in user space. We should strive to come up with an API
> that does the same thing regardless of the underlying hardware
> implementation.
> 
> Look at the proposed API, it basically says "Make the clock recovered
> from eth0 available on pin 1". If user space issues this command on
> different systems, it will mean different things, based on the
> underlying design of the hardware and the connection of the pin: To
> "DPLL/FPGA/anything else".

The link to the right DPLL input can be easily added later by adding
the new attribute to those commands. This has a big advantage
over the other model, as it is scalable to multiple different devices
serviced by different drivers, as it doesn't have the issue we have with
linking objects in kernel...

> Contrast that with an API that says "Set the source of EEC X to the
> clock recovered from eth0". This API is well defined and does the same
> thing across different systems.
> 
> Lets assume that these patches are merged as-is and tomorrow we get
> another implementation of these two ethtool operations in a different
> driver. We can't tell if these are used to feed the recovered clock into
> an EEC like ice does or enable some other use case that we never
> intended to enable.

...that issue is caused by the randomness and scalability of that 
infrastructure. At the time system boots it enumerates devices and
initializes them in  "random" order depending on the current system 
configuration which makes it impossible to code any co-driver 
dependencies, as it's impossible to guess what identifier the kernel
assigned to the other device that's connected to the same DPLL.

> Even if all the implementations use this API to feed the EEC, consider
> how difficult it is going to be for user space to use it. Ideally, user
> space should be able to query the state of the EEC and its source via an
> EEC object in a single command. With the current approach in which we
> have some amorphic object called "DPLL" that is only aware of pins and
> not netdevs, it's going to be very hard. User space will see that the
> DPLL is locked to the clock fed via pin 1. How user space is supposed to
> understand what is the source of this clock? Issue RCLK_GET dump and
> check for matching pin index that is enabled? User space has no reason
> to do it given that it doesn't even know that the source is a netdev.

Userspace tool will always know what ports it works with - it will come
from the config file, as - even if you have a dozen ports linked to a single
DPLL you may decide that you don't want to send ESMC messages
on some of them - just like we do with ptp4l.

The amorphic DPLL device has much more uses that a dedicated EEC
object and if we know what pin the DPLL uses and what is the link
between the RCLK output and the DPLL input it'll be very easy to
figure out what's going on.

The dump will also not be needed, as we know who we set as the best
source after receiving the ESMC packets from neighbors.

> >
> > I don't agree with the statement that we must have EEC object first,
> > as we can already configure different frequency sources using different
> > subsystems.
> 
> Regardless of all the above technical arguments, I think that these
> patches should not be merged now based on common sense alone. Not only
> these patches are of very limited use without an EEC object, they also
> prevent us from making changes to the API when such an object is
> introduced.

And I see a big value of merging them in as that would enable upstreaming
of the userspace tools that will explain a lot and close many gaps.

> > The source of signal should be separated from its consumer.
> 
> If it is completely separated (despite being hardwired on the board),
> then user space does not know how the signal is used when it issues the
> command. Is this signal fed into an EEC that controls that transmission
> rate of other netdev? Is this signal fed into an FPGA that blinks a led?

It's the same thing with frequency pins of the PTP subsystem. Userspace
tool can enable them, but has no idea what's on the other end of them.

> >
> > > With these four patches alone, user space doesn't know how many EECs
> > > there are in the system, it doesn't know the mapping from netdev to EEC,
> > > it doesn't know the state of the EEC, it doesn't know which source is
> > > chosen in case more than one source is enabled. Patch #3 tries to work
> > > around it by having ice print to kernel log, when the information should
> > > really be exposed via the EEC object.
> >
> > The goal of them is to add API for recovered clocks - not for EECs.
> 
> What do you mean by "not for EECs"? The file is called
> "net/ethtool/synce.c", if the signal is not being fed into an EEC then
> into what? It is unclear what kind of back doors this API will open.

Can be a DPLL of the radio part of the device, or different devices that
expects frequency reference.
If it makes it better we can move that to net/ethtool/rclk.c.

> > This part is there for observability and will still be there when EEC
> > is in place.  Those will need to be addressed by the DPLL subsystem.
> 
> If it is it only meant for observability, then why these messages are
> emitted as warnings to the kernel log? Regardless, the user API should
> be designed with observability in mind so that you wouldn't need to rely
> on prints to the kernel log.

You're right - need to change that to dev_info.
And yes - in the final form userspace tools will pull EEC's info from the DPLL
subsystem - this prints the state of 2 DPLLs and is designed for debuggability.

> >
> > > +		dev_warn(ice_pf_to_dev(pf),
> > > +			 "<DPLL%i> state changed to: %d, pin %d",
> > > +			 ICE_CGU_DPLL_SYNCE,
> > > +			 pf->synce_dpll_state,
> > > +			 pin);
> > >
> > > >
> > > > Further work is required to add the DPLL subsystem, link it to the
> > > > netdev object and create API to read the EEC DPLL state.
> > >
> > > When the EEC object materializes, we might find out that this API needs
> > > to be changed / reworked / removed, but we won't be able to do that
> > > given it's uAPI. I don't know where the confidence that it won't happen
> > > stems from when there are so many question marks around this new
> > > object.
> >
> > This API follows the functionality of other frequency outputs that exist
> > in the kernel, like PTP period file for frequency output of PTP clock
> > or other GPIOs. I highly doubt it'll change - we may extend it to add
> mapping
> > between pins, but like I indicated - this will not always be known to SW.
> >
> > Regards
> > Maciek

^ permalink raw reply	[flat|nested] 30+ messages in thread
* RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
  2021-12-17  0:01           ` [Intel-wired-lan] " Kubalewski, Arkadiusz
@ 2022-01-19 22:33             ` Kubalewski, Arkadiusz
  -1 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-01-19 22:33 UTC (permalink / raw)
  To: Vadim Fedorenko, Machnikowski, Maciej, Jakub Kicinski
  Cc: netdev, intel-wired-lan, richardcochran, Byagowi, Ahmad, Nguyen,
	Anthony L, davem, linux-kselftest, idosch, mkubecek, saeed,
	michael.chan, petrm

>>On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>>>> -----Original Message-----
>>>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>> Sent: poniedziałek, 13 grudnia 2021 09:54
>>>> To: Jakub Kicinski <kuba@kernel.org>
>>>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; 
>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
>>>> richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
>>>> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; 
>>>> linux-kselftest@vger.kernel.org; idosch@idosch.org; 
>>>> mkubecek@suse.cz; saeed@kernel.org; michael.chan@broadcom.com; 
>>>> petrm@nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>>>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for 
>>>> RClocks
>>>>
>>>>> -----Original Message-----
>>>>> From: Jakub Kicinski <kuba@kernel.org>
>>>>> Sent: Friday, December 10, 2021 5:17 PM
>>>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>>> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; 
>>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
>>>>> richardcochran@gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
>>>>> Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; linux- 
>>>>> kselftest@vger.kernel.org; idosch@idosch.org; mkubecek@suse.cz; 
>>>>> saeed@kernel.org; michael.chan@broadcom.com; petrm@nvidia.com; 
>>>>> Vadim Fedorenko <vfedorenko@novek.ru>
>>>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for 
>>>>> RClocks
>>>>>
>>>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>>>> Synchronous Ethernet networks use a physical layer clock to 
>>>>>> syntonize the frequency across different network elements.
>>>>>>
>>>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an 
>>>>>> Ethernet Equipment Clock (EEC) and have the ability to synchronize 
>>>>>> to reference frequency sources.
>>>>>>
>>>>>> This patch series is a prerequisite for EEC object and adds 
>>>>>> ability to enable recovered clocks in the physical layer of the netdev object.
>>>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>>>
>>>>>> Further work is required to add the DPLL subsystem, link it to the 
>>>>>> netdev object and create API to read the EEC DPLL state.
>>>>>
>>>>> You missed CCing Vadim. I guess Ccing the right people may be right 
>>>>> up there with naming things as the hardest things in SW development..
>>>>>
>>>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>>>
>>>> Sounds about right :) thanks for adding Vadim!
>>>>
>>> 
>>> Good day Vadim,
>>> 
>>> Can we help on the new PLL interfaces?
>>> I can start some works related to that, although would need a 
>>> guidance from the expert.
>>> Where to place it?
>>> What in-kernel interfaces to use?
>>> Any other high level tips that could be useful?
>>> Or if you already started some work, could you please share some 
>>> information?
>>> 
>>Hi!
>>
>>I'm going to publish RFC till the end of the week and we will be able 
>>to continue discussion via this mailing list. I think that netlink is a 
>>good option for in-kernel interface and is easy to implement.
>>
>
>Oh, that sounds great!
>Thank you!
>

Good day Vadim,

Hope you are well!
I have been trying to find a new RFC on mailing list, but could not find it.
I guess, it was not submitted yet?
Could you please share your current schedule for this task?
Or maybe we could help somehow?

^ permalink raw reply	[flat|nested] 30+ messages in thread
* [Intel-wired-lan] [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
@ 2022-01-19 22:33             ` Kubalewski, Arkadiusz
  0 siblings, 0 replies; 30+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-01-19 22:33 UTC (permalink / raw)
  To: intel-wired-lan

>>On 15.12.2021 12:14, Kubalewski, Arkadiusz wrote:
>>>> -----Original Message-----
>>>> From: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>> Sent: poniedzia?ek, 13 grudnia 2021 09:54
>>>> To: Jakub Kicinski <kuba@kernel.org>
>>>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; 
>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
>>>> richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
>>>> Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; 
>>>> linux-kselftest at vger.kernel.org; idosch at idosch.org; 
>>>> mkubecek at suse.cz; saeed at kernel.org; michael.chan at broadcom.com; 
>>>> petrm at nvidia.com; Vadim Fedorenko <vfedorenko@novek.ru>
>>>> Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for 
>>>> RClocks
>>>>
>>>>> -----Original Message-----
>>>>> From: Jakub Kicinski <kuba@kernel.org>
>>>>> Sent: Friday, December 10, 2021 5:17 PM
>>>>> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
>>>>> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; 
>>>>> Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; 
>>>>> richardcochran at gmail.com; Byagowi, Ahmad <abyagowi@fb.com>; Nguyen, 
>>>>> Anthony L <anthony.l.nguyen@intel.com>; davem at davemloft.net; linux- 
>>>>> kselftest at vger.kernel.org; idosch at idosch.org; mkubecek at suse.cz; 
>>>>> saeed at kernel.org; michael.chan at broadcom.com; petrm at nvidia.com; 
>>>>> Vadim Fedorenko <vfedorenko@novek.ru>
>>>>> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for 
>>>>> RClocks
>>>>>
>>>>> On Fri, 10 Dec 2021 14:45:46 +0100 Maciej Machnikowski wrote:
>>>>>> Synchronous Ethernet networks use a physical layer clock to 
>>>>>> syntonize the frequency across different network elements.
>>>>>>
>>>>>> Basic SyncE node defined in the ITU-T G.8264 consist of an 
>>>>>> Ethernet Equipment Clock (EEC) and have the ability to synchronize 
>>>>>> to reference frequency sources.
>>>>>>
>>>>>> This patch series is a prerequisite for EEC object and adds 
>>>>>> ability to enable recovered clocks in the physical layer of the netdev object.
>>>>>> Recovered clocks can be used as one of the reference signal by the EEC.
>>>>>>
>>>>>> Further work is required to add the DPLL subsystem, link it to the 
>>>>>> netdev object and create API to read the EEC DPLL state.
>>>>>
>>>>> You missed CCing Vadim. I guess Ccing the right people may be right 
>>>>> up there with naming things as the hardest things in SW development..
>>>>>
>>>>> Anyway, Vadim - do you have an ETA on the first chunk of the PLL work?
>>>>
>>>> Sounds about right :) thanks for adding Vadim!
>>>>
>>> 
>>> Good day Vadim,
>>> 
>>> Can we help on the new PLL interfaces?
>>> I can start some works related to that, although would need a 
>>> guidance from the expert.
>>> Where to place it?
>>> What in-kernel interfaces to use?
>>> Any other high level tips that could be useful?
>>> Or if you already started some work, could you please share some 
>>> information?
>>> 
>>Hi!
>>
>>I'm going to publish RFC till the end of the week and we will be able 
>>to continue discussion via this mailing list. I think that netlink is a 
>>good option for in-kernel interface and is easy to implement.
>>
>
>Oh, that sounds great!
>Thank you!
>

Good day Vadim,

Hope you are well!
I have been trying to find a new RFC on mailing list, but could not find it.
I guess, it was not submitted yet?
Could you please share your current schedule for this task?
Or maybe we could help somehow?

^ permalink raw reply	[flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-01-19 22:33 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-10 13:45 [PATCH v5 net-next 0/4] Add ethtool interface for RClocks Maciej Machnikowski
2021-12-10 13:45 ` [Intel-wired-lan] " Maciej Machnikowski
2021-12-10 13:45 ` [PATCH v5 net-next 1/4] ice: add support detecting features based on netlist Maciej Machnikowski
2021-12-10 13:45   ` [Intel-wired-lan] " Maciej Machnikowski
2021-12-10 13:45 ` [PATCH v5 net-next 2/4] ethtool: Add ability to configure recovered clock for SyncE feature Maciej Machnikowski
2021-12-10 13:45   ` [Intel-wired-lan] " Maciej Machnikowski
2021-12-10 13:45 ` [PATCH v5 net-next 3/4] ice: add support for monitoring SyncE DPLL state Maciej Machnikowski
2021-12-10 13:45   ` [Intel-wired-lan] " Maciej Machnikowski
2021-12-10 13:45 ` [PATCH v5 net-next 4/4] ice: add support for recovered clocks Maciej Machnikowski
2021-12-10 13:45   ` [Intel-wired-lan] " Maciej Machnikowski
2021-12-10 16:16 ` [PATCH v5 net-next 0/4] Add ethtool interface for RClocks Jakub Kicinski
2021-12-10 16:16   ` [Intel-wired-lan] " Jakub Kicinski
2021-12-13  8:53   ` Machnikowski, Maciej
2021-12-13  8:53     ` [Intel-wired-lan] " Machnikowski, Maciej
2021-12-15 12:14     ` Kubalewski, Arkadiusz
2021-12-15 12:14       ` [Intel-wired-lan] " Kubalewski, Arkadiusz
2021-12-15 22:27       ` Vadim Fedorenko
2021-12-15 22:27         ` [Intel-wired-lan] " Vadim Fedorenko
2021-12-17  0:01         ` Kubalewski, Arkadiusz
2021-12-17  0:01           ` [Intel-wired-lan] " Kubalewski, Arkadiusz
2022-01-19 22:33           ` Kubalewski, Arkadiusz
2022-01-19 22:33             ` [Intel-wired-lan] " Kubalewski, Arkadiusz
2021-12-12 11:47 ` Ido Schimmel
2021-12-12 11:47   ` [Intel-wired-lan] " Ido Schimmel
2021-12-15 12:13   ` Machnikowski, Maciej
2021-12-15 12:13     ` [Intel-wired-lan] " Machnikowski, Maciej
2021-12-20 13:50     ` Ido Schimmel
2021-12-20 13:50       ` [Intel-wired-lan] " Ido Schimmel
2021-12-21 22:12       ` Machnikowski, Maciej
2021-12-21 22:12         ` [Intel-wired-lan] " Machnikowski, Maciej

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.