[Intel-wired-lan] [PATCH iwl-next 0/4] ice: refactor PTP feature flags

[Intel-wired-lan] [PATCH iwl-next 0/4] ice: refactor PTP feature flags

[Jacob Keller]

This series refactors and extends the feature flag detection for a couple of
PTP feature flags. This includes ICE_F_GNSS and ICE_F_SMA_CTRL. Instead of
blindly assuming the feature is enabled on all E810-T devices, check the
netlist to confirm that the feature is supported on that device and
platform.

For SMA control, this allows the driver to fallback to the fixed pin
configuration that is supported by default E810 configurations when the SMA
control is not accessible.

For GNSS, this ensures that we do not attempt to read the GNSS portion of
the device if its not present, avoiding some unnecessary warning messages.

For ICE_F_SMA_CTRL this could be seen as a fix, but given the scope of the
code I decided that its next material. I think of it more as extending the
feature capability to support pins on more platforms.

Jacob Keller (4):
  ice: remove ICE_F_PTP_EXTTS feature flag
  ice: don't enable PTP related capabilities on non-owner PFs
  ice: check the netlist before enabling ICE_F_SMA_CTRL
  ice: check netlist before enabling ICE_F_GNSS

 drivers/net/ethernet/intel/ice/ice.h          |  1 -
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  8 +++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      | 12 +++--
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  4 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 31 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  2 +
 8 files changed, 96 insertions(+), 11 deletions(-)


base-commit: 361b86237e1afbf2c3be7cb604b6aac6f8b8c38c
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 1/4] ice: remove ICE_F_PTP_EXTTS feature flag

[Jacob Keller]

The ICE_F_PTP_EXTTS feature flag is ostensibly intended to support checking
whether the device supports external timestamp pins. It is only checked in
E810-specific code flows, and is enabled for all E810-based devices. E822
and E823 flows unconditionally enable external timestamp support.

This makes the feature flag meaningless, as it is always enabled. Just
unconditionally enable support for external timestamp pins and remove this
unnecessary flag.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     | 1 -
 drivers/net/ethernet/intel/ice/ice_lib.c | 1 -
 drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +---
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 5d307bacf7c6..013ea8970fbc 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -199,7 +199,6 @@
 
 enum ice_feature {
 	ICE_F_DSCP,
-	ICE_F_PTP_EXTTS,
 	ICE_F_SMA_CTRL,
 	ICE_F_GNSS,
 	ICE_F_ROCE_LAG,
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 201570cd2e0b..5dfcb824f817 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3986,7 +3986,6 @@ 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);
-		ice_set_feature_support(pf, ICE_F_PTP_EXTTS);
 		if (ice_is_e810t(&pf->hw)) {
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
 			if (ice_gnss_is_gps_present(&pf->hw))
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 97b8581ae931..9524fcea9ae9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2205,9 +2205,7 @@ static void
 ice_ptp_setup_pins_e810(struct ice_pf *pf, struct ptp_clock_info *info)
 {
 	info->n_per_out = N_PER_OUT_E810;
-
-	if (ice_is_feature_supported(pf, ICE_F_PTP_EXTTS))
-		info->n_ext_ts = N_EXT_TS_E810;
+	info->n_ext_ts = N_EXT_TS_E810;
 
 	if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL)) {
 		info->n_ext_ts = N_EXT_TS_E810;
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 2/4] ice: don't enable PTP related capabilities on non-owner PFs

[Jacob Keller]

The ice driver currently sets feature flags for certain PTP related
capabilities based on whether the device has support for the feature. Avoid
enabling these capabilities except on the ports which own the timer. This
ensures that the driver never attempts to access the features except on the
ports designated as controlling the main timer functionality.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 5dfcb824f817..f29ff54383b5 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3986,6 +3986,9 @@ 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 we don't own the timer - don't enable other caps */
+		if (!ice_pf_src_tmr_owned(pf))
+			break;
 		if (ice_is_e810t(&pf->hw)) {
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
 			if (ice_gnss_is_gps_present(&pf->hw))
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Jacob Keller]

Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
E810-T based devices. In some cases, the SMA control access is not
available in the netlist firmware component. In such a case, the driver
will fail to setup the SMA pins. When this happens, the driver prints
"Failed to configure E810-T SMA pin control" and forcibly disables all PTP
pin configuration support.

This results in failure to use even the fixed pin capabilities of standard
E810 devices, resulting in reduced functionality.

To avoid this, check the netlist for the SMA control module before enabling
the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
feature will not be enabled. In this case, the driver flow for enabling
periodic outputs and external timestamps will fall back to the standard
fixed pin configuration.

This allows supporting the software defined pins on a wider array of
platforms.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
 6 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 90616750e779..82c4daf0a825 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1367,6 +1367,7 @@ 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_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)
@@ -1403,8 +1404,9 @@ 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_C827	0x31
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
 	return false;
 }
 
+#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 0 is returned, -ENOENT otherwise. If
+ * node_handle provided, it would be set to found node handle.
+ */
+int
+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;
+	u16 rec_node_handle;
+	u8 idx;
+
+	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
+		int status;
+
+		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 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
 /**
  * ice_aq_list_caps - query function/device capabilities
  * @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 2250a9c5f61e..f7178a5686a5 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
 		    struct ice_sq_cd *cd);
 bool ice_is_pf_c827(struct ice_hw *hw);
 int
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle);
+int
 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);
 int
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index f29ff54383b5..4ac8998cb964 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
 		/* If we don't own the timer - don't enable other caps */
 		if (!ice_pf_src_tmr_owned(pf))
 			break;
-		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_e810t(&pf->hw)) {
 			if (ice_gnss_is_gps_present(&pf->hw))
 				ice_set_feature_support(pf, ICE_F_GNSS);
 		}
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index fd19afaf9c85..bd3f32bfbc78 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle)
 	return 0;
 }
 
+/**
+ * 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;
+}
+
 /**
  * ice_read_sma_ctrl_e810t
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 4e3c1382c477..3768e7a01920 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8 port);
 int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
 
 /* E810 family functions */
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
 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);
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 4/4] ice: check netlist before enabling ICE_F_GNSS

[Jacob Keller]

Similar to the change made for ICE_F_SMA_CTRL, check the netlist before
enabling support for ICE_F_GNSS. This ensures that the driver only enables
the GNSS feature on devices which actually have the feature enabled in the
firmware device configuration.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adminq_cmd.h |  2 ++
 drivers/net/ethernet/intel/ice/ice_lib.c        |  7 +++----
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c     | 15 +++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h     |  1 +
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 82c4daf0a825..2f0d4bffe210 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1368,6 +1368,7 @@ struct ice_aqc_link_topo_params {
 #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_MUX	10
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_GPS		11
 #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)
@@ -1407,6 +1408,7 @@ struct ice_aqc_get_link_topo {
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			0x31
 #define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX		0x47
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS			0x48
 	u8 rsvd[9];
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 4ac8998cb964..24a30b55c7ff 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3991,10 +3991,9 @@ void ice_init_feature_support(struct ice_pf *pf)
 			break;
 		if (ice_is_clock_mux_present_e810t(&pf->hw))
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
-		if (ice_is_e810t(&pf->hw)) {
-			if (ice_gnss_is_gps_present(&pf->hw))
-				ice_set_feature_support(pf, ICE_F_GNSS);
-		}
+		if (ice_is_gps_present_e810t(&pf->hw) &&
+		    ice_gnss_is_gps_present(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_GNSS);
 		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 bd3f32bfbc78..455d7a15de26 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -3034,6 +3034,21 @@ bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
 	return true;
 }
 
+/**
+ * ice_is_gps_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the GPS generic device is present in the netlist
+ */
+bool ice_is_gps_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_GPS,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS, NULL))
+		return false;
+
+	return true;
+}
+
 /**
  * ice_read_sma_ctrl_e810t
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 3768e7a01920..4399338b7347 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -196,6 +196,7 @@ int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
 
 /* E810 family functions */
 bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
+bool ice_is_gps_present_e810t(struct ice_hw *hw);
 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);
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 0/4] ice: refactor PTP feature flags

[Jacob Keller]

This series refactors and extends the feature flag detection for a couple of
PTP feature flags. This includes ICE_F_GNSS and ICE_F_SMA_CTRL. Instead of
blindly assuming the feature is enabled on all E810-T devices, check the
netlist to confirm that the feature is supported on that device and
platform.

For SMA control, this allows the driver to fallback to the fixed pin
configuration that is supported by default E810 configurations when the SMA
control is not accessible.

For GNSS, this ensures that we do not attempt to read the GNSS portion of
the device if its not present, avoiding some unnecessary warning messages.

For ICE_F_SMA_CTRL this could be seen as a fix, but given the scope of the
code I decided that its next material. I think of it more as extending the
feature capability to support pins on more platforms.

Jacob Keller (4):
  ice: remove ICE_F_PTP_EXTTS feature flag
  ice: don't enable PTP related capabilities on non-owner PFs
  ice: check the netlist before enabling ICE_F_SMA_CTRL
  ice: check netlist before enabling ICE_F_GNSS

 drivers/net/ethernet/intel/ice/ice.h          |  1 -
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  8 +++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      | 12 +++--
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  4 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 31 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  2 +
 8 files changed, 96 insertions(+), 11 deletions(-)


base-commit: 361b86237e1afbf2c3be7cb604b6aac6f8b8c38c
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 1/4] ice: remove ICE_F_PTP_EXTTS feature flag

[Jacob Keller]

The ICE_F_PTP_EXTTS feature flag is ostensibly intended to support checking
whether the device supports external timestamp pins. It is only checked in
E810-specific code flows, and is enabled for all E810-based devices. E822
and E823 flows unconditionally enable external timestamp support.

This makes the feature flag meaningless, as it is always enabled. Just
unconditionally enable support for external timestamp pins and remove this
unnecessary flag.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     | 1 -
 drivers/net/ethernet/intel/ice/ice_lib.c | 1 -
 drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +---
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 5d307bacf7c6..013ea8970fbc 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -199,7 +199,6 @@
 
 enum ice_feature {
 	ICE_F_DSCP,
-	ICE_F_PTP_EXTTS,
 	ICE_F_SMA_CTRL,
 	ICE_F_GNSS,
 	ICE_F_ROCE_LAG,
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 201570cd2e0b..5dfcb824f817 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3986,7 +3986,6 @@ 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);
-		ice_set_feature_support(pf, ICE_F_PTP_EXTTS);
 		if (ice_is_e810t(&pf->hw)) {
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
 			if (ice_gnss_is_gps_present(&pf->hw))
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 97b8581ae931..9524fcea9ae9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2205,9 +2205,7 @@ static void
 ice_ptp_setup_pins_e810(struct ice_pf *pf, struct ptp_clock_info *info)
 {
 	info->n_per_out = N_PER_OUT_E810;
-
-	if (ice_is_feature_supported(pf, ICE_F_PTP_EXTTS))
-		info->n_ext_ts = N_EXT_TS_E810;
+	info->n_ext_ts = N_EXT_TS_E810;
 
 	if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL)) {
 		info->n_ext_ts = N_EXT_TS_E810;
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 2/4] ice: don't enable PTP related capabilities on non-owner PFs

[Jacob Keller]

The ice driver currently sets feature flags for certain PTP related
capabilities based on whether the device has support for the feature. Avoid
enabling these capabilities except on the ports which own the timer. This
ensures that the driver never attempts to access the features except on the
ports designated as controlling the main timer functionality.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 5dfcb824f817..f29ff54383b5 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3986,6 +3986,9 @@ 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 we don't own the timer - don't enable other caps */
+		if (!ice_pf_src_tmr_owned(pf))
+			break;
 		if (ice_is_e810t(&pf->hw)) {
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
 			if (ice_gnss_is_gps_present(&pf->hw))
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Jacob Keller]

Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
E810-T based devices. In some cases, the SMA control access is not
available in the netlist firmware component. In such a case, the driver
will fail to setup the SMA pins. When this happens, the driver prints
"Failed to configure E810-T SMA pin control" and forcibly disables all PTP
pin configuration support.

This results in failure to use even the fixed pin capabilities of standard
E810 devices, resulting in reduced functionality.

To avoid this, check the netlist for the SMA control module before enabling
the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
feature will not be enabled. In this case, the driver flow for enabling
periodic outputs and external timestamps will fall back to the standard
fixed pin configuration.

This allows supporting the software defined pins on a wider array of
platforms.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
 drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
 drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
 6 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 90616750e779..82c4daf0a825 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1367,6 +1367,7 @@ 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_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)
@@ -1403,8 +1404,9 @@ 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_C827	0x31
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
 	return false;
 }
 
+#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 0 is returned, -ENOENT otherwise. If
+ * node_handle provided, it would be set to found node handle.
+ */
+int
+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;
+	u16 rec_node_handle;
+	u8 idx;
+
+	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
+		int status;
+
+		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 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
 /**
  * ice_aq_list_caps - query function/device capabilities
  * @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 2250a9c5f61e..f7178a5686a5 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
 		    struct ice_sq_cd *cd);
 bool ice_is_pf_c827(struct ice_hw *hw);
 int
+ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
+		      u16 *node_handle);
+int
 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);
 int
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index f29ff54383b5..4ac8998cb964 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
 		/* If we don't own the timer - don't enable other caps */
 		if (!ice_pf_src_tmr_owned(pf))
 			break;
-		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_e810t(&pf->hw)) {
 			if (ice_gnss_is_gps_present(&pf->hw))
 				ice_set_feature_support(pf, ICE_F_GNSS);
 		}
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index fd19afaf9c85..bd3f32bfbc78 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle)
 	return 0;
 }
 
+/**
+ * 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;
+}
+
 /**
  * ice_read_sma_ctrl_e810t
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 4e3c1382c477..3768e7a01920 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8 port);
 int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
 
 /* E810 family functions */
+bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
 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);
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH iwl-next 4/4] ice: check netlist before enabling ICE_F_GNSS

[Jacob Keller]

Similar to the change made for ICE_F_SMA_CTRL, check the netlist before
enabling support for ICE_F_GNSS. This ensures that the driver only enables
the GNSS feature on devices which actually have the feature enabled in the
firmware device configuration.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adminq_cmd.h |  2 ++
 drivers/net/ethernet/intel/ice/ice_lib.c        |  7 +++----
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c     | 15 +++++++++++++++
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h     |  1 +
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 82c4daf0a825..2f0d4bffe210 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1368,6 +1368,7 @@ struct ice_aqc_link_topo_params {
 #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_MUX	10
+#define ICE_AQC_LINK_TOPO_NODE_TYPE_GPS		11
 #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)
@@ -1407,6 +1408,7 @@ struct ice_aqc_get_link_topo {
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
 #define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			0x31
 #define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX		0x47
+#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS			0x48
 	u8 rsvd[9];
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 4ac8998cb964..24a30b55c7ff 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3991,10 +3991,9 @@ void ice_init_feature_support(struct ice_pf *pf)
 			break;
 		if (ice_is_clock_mux_present_e810t(&pf->hw))
 			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
-		if (ice_is_e810t(&pf->hw)) {
-			if (ice_gnss_is_gps_present(&pf->hw))
-				ice_set_feature_support(pf, ICE_F_GNSS);
-		}
+		if (ice_is_gps_present_e810t(&pf->hw) &&
+		    ice_gnss_is_gps_present(&pf->hw))
+			ice_set_feature_support(pf, ICE_F_GNSS);
 		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 bd3f32bfbc78..455d7a15de26 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -3034,6 +3034,21 @@ bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
 	return true;
 }
 
+/**
+ * ice_is_gps_present_e810t
+ * @hw: pointer to the hw struct
+ *
+ * Check if the GPS generic device is present in the netlist
+ */
+bool ice_is_gps_present_e810t(struct ice_hw *hw)
+{
+	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_GPS,
+				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS, NULL))
+		return false;
+
+	return true;
+}
+
 /**
  * ice_read_sma_ctrl_e810t
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 3768e7a01920..4399338b7347 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -196,6 +196,7 @@ int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
 
 /* E810 family functions */
 bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
+bool ice_is_gps_present_e810t(struct ice_hw *hw);
 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);
-- 
2.41.0.1.g9857a21e0017.dirty

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Jacob Keller]

On 8/15/2023 3:35 PM, Jacob Keller wrote:
> Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
> E810-T based devices. In some cases, the SMA control access is not
> available in the netlist firmware component. In such a case, the driver
> will fail to setup the SMA pins. When this happens, the driver prints
> "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
> pin configuration support.
> 
> This results in failure to use even the fixed pin capabilities of standard
> E810 devices, resulting in reduced functionality.
> 
> To avoid this, check the netlist for the SMA control module before enabling
> the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
> feature will not be enabled. In this case, the driver flow for enabling
> periodic outputs and external timestamps will fall back to the standard
> fixed pin configuration.
> 
> This allows supporting the software defined pins on a wider array of
> platforms.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
>  drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
>  drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
>  6 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 90616750e779..82c4daf0a825 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1367,6 +1367,7 @@ 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_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)
> @@ -1403,8 +1404,9 @@ 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_C827	0x31
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
>  	return false;
>  }
>  
> +#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 0 is returned, -ENOENT otherwise. If
> + * node_handle provided, it would be set to found node handle.
> + */
> +int
> +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;
> +	u16 rec_node_handle;
> +	u8 idx;
> +
> +	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
> +		int status;
> +
> +		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 0;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  /**
>   * ice_aq_list_caps - query function/device capabilities
>   * @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 2250a9c5f61e..f7178a5686a5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
>  		    struct ice_sq_cd *cd);
>  bool ice_is_pf_c827(struct ice_hw *hw);
>  int
> +ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
> +		      u16 *node_handle);
> +int
>  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);
>  int
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index f29ff54383b5..4ac8998cb964 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
>  		/* If we don't own the timer - don't enable other caps */
>  		if (!ice_pf_src_tmr_owned(pf))
>  			break;
> -		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_e810t(&pf->hw)) {
>  			if (ice_gnss_is_gps_present(&pf->hw))
>  				ice_set_feature_support(pf, ICE_F_GNSS);
>  		}

This change also revealed a subtle issue with pin setup in ice_ptp.c,
where the E810-T device needs to use a different fallback value for the
pin count. I'll send a v2 of this series tomorrow with an additional
patch to fix that up.

Thanks,
Jake

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index fd19afaf9c85..bd3f32bfbc78 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle)
>  	return 0;
>  }
>  
> +/**
> + * 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;
> +}
> +
>  /**
>   * ice_read_sma_ctrl_e810t
>   * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 4e3c1382c477..3768e7a01920 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8 port);
>  int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
>  
>  /* E810 family functions */
> +bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
>  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);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 4/4] ice: check netlist before enabling ICE_F_GNSS

[Jacob Keller]

On 8/15/2023 3:35 PM, Jacob Keller wrote:
> Similar to the change made for ICE_F_SMA_CTRL, check the netlist before
> enabling support for ICE_F_GNSS. This ensures that the driver only enables
> the GNSS feature on devices which actually have the feature enabled in the
> firmware device configuration.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_adminq_cmd.h |  2 ++
>  drivers/net/ethernet/intel/ice/ice_lib.c        |  7 +++----
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c     | 15 +++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h     |  1 +
>  4 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 82c4daf0a825..2f0d4bffe210 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1368,6 +1368,7 @@ struct ice_aqc_link_topo_params {
>  #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_MUX	10
> +#define ICE_AQC_LINK_TOPO_NODE_TYPE_GPS		11
>  #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)
> @@ -1407,6 +1408,7 @@ struct ice_aqc_get_link_topo {
>  #define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
>  #define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			0x31
>  #define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX		0x47
> +#define ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS			0x48
>  	u8 rsvd[9];
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 4ac8998cb964..24a30b55c7ff 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3991,10 +3991,9 @@ void ice_init_feature_support(struct ice_pf *pf)
>  			break;
>  		if (ice_is_clock_mux_present_e810t(&pf->hw))
>  			ice_set_feature_support(pf, ICE_F_SMA_CTRL);
> -		if (ice_is_e810t(&pf->hw)) {
> -			if (ice_gnss_is_gps_present(&pf->hw))
> -				ice_set_feature_support(pf, ICE_F_GNSS);
> -		}
> +		if (ice_is_gps_present_e810t(&pf->hw) &&
> +		    ice_gnss_is_gps_present(&pf->hw))
> +			ice_set_feature_support(pf, ICE_F_GNSS);

The names for these two functions are incredibly similar, and I'm not
sure if I have a better name. @Karol, this is what we have in our
sourceforge driver now, but I'm not sure I like it.

I think I'll drop this patch from the series and we can investigate this
and sort things out a bit?

Thanks,
Jake

>  		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 bd3f32bfbc78..455d7a15de26 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -3034,6 +3034,21 @@ bool ice_is_clock_mux_present_e810t(struct ice_hw *hw)
>  	return true;
>  }
>  
> +/**
> + * ice_is_gps_present_e810t
> + * @hw: pointer to the hw struct
> + *
> + * Check if the GPS generic device is present in the netlist
> + */
> +bool ice_is_gps_present_e810t(struct ice_hw *hw)
> +{
> +	if (ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_GPS,
> +				  ICE_ACQ_GET_LINK_TOPO_NODE_NR_GEN_GPS, NULL))
> +		return false;
> +
> +	return true;
> +}
> +
>  /**
>   * ice_read_sma_ctrl_e810t
>   * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 3768e7a01920..4399338b7347 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -196,6 +196,7 @@ int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
>  
>  /* E810 family functions */
>  bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
> +bool ice_is_gps_present_e810t(struct ice_hw *hw);
>  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);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[kernel test robot]

Hi Jacob,

kernel test robot noticed the following build errors:

[auto build test ERROR on 361b86237e1afbf2c3be7cb604b6aac6f8b8c38c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/ice-remove-ICE_F_PTP_EXTTS-feature-flag/20230816-063804
base:   361b86237e1afbf2c3be7cb604b6aac6f8b8c38c
patch link:    https://lore.kernel.org/r/20230815223551.1238091-4-jacob.e.keller%40intel.com
patch subject: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL
config: x86_64-buildonly-randconfig-r001-20230816 (https://download.01.org/0day-ci/archive/20230816/202308160903.rp10O1O3-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230816/202308160903.rp10O1O3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308160903.rp10O1O3-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/arcnet/com20020_cs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/broadcom/bcm4908_enet.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/freescale/enetc/fsl-enetc-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/litex/litex_liteeth.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/dummy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/eql.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/appletalk/ipddp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/plip/plip.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/sungem_phy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ieee802154/fakelb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_pruss.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/yenta_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/libcomposite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ss_lb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_ether.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ncm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_rndis.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_fs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_uvc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_midi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/legacy/g_zero.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-qup.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/common/uvc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-async.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-fwnode.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/radio/si470x/radio-si470x-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/asus_atk0110.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/intel_soc_dts_iosf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/watchdog/menz69_wdt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cpuidle/cpuidle-haltpoll.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/blink/leds-bcm63138.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/flash/leds-rt4505.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/fsl_imx8_ddr_perf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwtracing/intel_th/intel_th_msu_sink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_brcm_nvram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mm-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mq-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mn-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/greybus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/core/snd-pcm-dmaengine.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-ab8500-codec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-sigmadsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-wm-adsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-i2s.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-pdm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-pci.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-mach.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/snd-acp-config.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/fsl/imx-pcm-dma.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/qcom/snd-soc-qcom-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/ac97_bus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/bytestream-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/dma-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/inttype-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/record-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kmemleak/kmemleak-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/802/stp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/802/mrp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_hfsc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_ingress.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_teql.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_prio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_netem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_drr.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_ets.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_choke.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_etf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_taprio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ipip.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ah4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/xfrm4_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/tunnel4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/inet_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/tcp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/udp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/raw_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/sit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/8021q/8021q.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/vmw_vsock/vsock_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/bridge/bridge.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/atm/atm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sctp/sctp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/caif.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/chnl_net.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/caif_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/6lowpan/ieee802154_6lowpan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/ieee802154_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nci/nci.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nci/nci_spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nfc_digital.o
>> ERROR: modpost: "ice_is_clock_mux_present_e810t" [drivers/net/ethernet/intel/ice/ice.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 4/4] ice: check netlist before enabling ICE_F_GNSS

[kernel test robot]

Hi Jacob,

kernel test robot noticed the following build errors:

[auto build test ERROR on 361b86237e1afbf2c3be7cb604b6aac6f8b8c38c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacob-Keller/ice-remove-ICE_F_PTP_EXTTS-feature-flag/20230816-063804
base:   361b86237e1afbf2c3be7cb604b6aac6f8b8c38c
patch link:    https://lore.kernel.org/r/20230815223551.1238091-5-jacob.e.keller%40intel.com
patch subject: [Intel-wired-lan] [PATCH iwl-next 4/4] ice: check netlist before enabling ICE_F_GNSS
config: x86_64-buildonly-randconfig-r001-20230816 (https://download.01.org/0day-ci/archive/20230816/202308161024.wzGJ6bHC-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230816/202308161024.wzGJ6bHC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308161024.wzGJ6bHC-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/broadcom/bcm4908_enet.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/freescale/enetc/fsl-enetc-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ethernet/litex/litex_liteeth.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/dummy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/eql.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/appletalk/ipddp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/plip/plip.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/sungem_phy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/net/ieee802154/fakelb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_pruss.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/yenta_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/libcomposite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ss_lb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_ether.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ncm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_rndis.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_fs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_uvc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_midi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/legacy/g_zero.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-qup.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/common/uvc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-async.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-fwnode.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/radio/si470x/radio-si470x-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/asus_atk0110.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/intel_soc_dts_iosf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/watchdog/menz69_wdt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cpuidle/cpuidle-haltpoll.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/blink/leds-bcm63138.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/flash/leds-rt4505.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/fsl_imx8_ddr_perf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwtracing/intel_th/intel_th_msu_sink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_brcm_nvram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mm-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mq-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mn-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/greybus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/core/snd-pcm-dmaengine.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-ab8500-codec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-sigmadsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-wm-adsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-i2s.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-pdm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-pci.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/acp/snd-acp-mach.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/amd/snd-acp-config.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/fsl/imx-pcm-dma.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/qcom/snd-soc-qcom-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/ac97_bus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/bytestream-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/dma-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/inttype-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/record-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kmemleak/kmemleak-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/802/stp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/802/mrp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_hfsc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_ingress.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_teql.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_prio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_netem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_drr.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_ets.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_choke.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_etf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/sch_taprio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ipip.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ah4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/xfrm4_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/tunnel4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/inet_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/tcp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/udp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/raw_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/sit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/8021q/8021q.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/vmw_vsock/vsock_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/bridge/bridge.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/atm/atm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sctp/sctp_diag.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/caif.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/chnl_net.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/caif/caif_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/6lowpan/ieee802154_6lowpan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/ieee802154_socket.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nci/nci.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nci/nci_spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/nfc/nfc_digital.o
ERROR: modpost: "ice_is_clock_mux_present_e810t" [drivers/net/ethernet/intel/ice/ice.ko] undefined!
>> ERROR: modpost: "ice_is_gps_present_e810t" [drivers/net/ethernet/intel/ice/ice.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Przemek Kitszel]

On 8/16/23 00:35, Jacob Keller wrote:
> Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
> E810-T based devices. In some cases, the SMA control access is not
> available in the netlist firmware component. In such a case, the driver
> will fail to setup the SMA pins. When this happens, the driver prints
> "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
> pin configuration support.
> 
> This results in failure to use even the fixed pin capabilities of standard
> E810 devices, resulting in reduced functionality.
> 
> To avoid this, check the netlist for the SMA control module before enabling
> the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
> feature will not be enabled. In this case, the driver flow for enabling
> periodic outputs and external timestamps will fall back to the standard
> fixed pin configuration.
> 
> This allows supporting the software defined pins on a wider array of
> platforms.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Overall it's a nice series!

> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
>   drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
>   drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
>   6 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 90616750e779..82c4daf0a825 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1367,6 +1367,7 @@ 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_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)
> @@ -1403,8 +1404,9 @@ 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_C827	0x31
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
>   	return false;
>   }
>   
> +#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 0 is returned, -ENOENT otherwise. If
> + * node_handle provided, it would be set to found node handle.
> + */
> +int
> +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;
> +	u16 rec_node_handle;

I see that you are using separate variable to 'do not touch' 
@node_handle param if it does not need to be updated.
But perhaps you could consider to just pass @node_handle in place of 
@rec_node_handle below, and have less code?
I do not see any non-null usage of the field anyway.

(rationale: our code is so self-similar that I needed to check wheater 
you are basing-of recent changes by Jan&Karol or re-doing them ;)
answer: we are fine here :)).

> +	u8 idx;
> +
> +	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
> +		int status;
> +
> +		memset(&cmd, 0, sizeof(cmd));
> +
> +		cmd.addr.topo_params.node_type_ctx =
> +			(node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S);

I would FIELD_PREP() here

and perhaps convert @cmd scope to inside the loop, that would enable = { 
.addr.topo_params = { ... } } declaration

> +		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 0;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
>   /**
>    * ice_aq_list_caps - query function/device capabilities
>    * @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 2250a9c5f61e..f7178a5686a5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
>   		    struct ice_sq_cd *cd);
>   bool ice_is_pf_c827(struct ice_hw *hw);
>   int
> +ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number,
> +		      u16 *node_handle);
> +int
>   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);
>   int
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index f29ff54383b5..4ac8998cb964 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
>   		/* If we don't own the timer - don't enable other caps */
>   		if (!ice_pf_src_tmr_owned(pf))
>   			break;
> -		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_e810t(&pf->hw)) {
>   			if (ice_gnss_is_gps_present(&pf->hw))
>   				ice_set_feature_support(pf, ICE_F_GNSS);
>   		}
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index fd19afaf9c85..bd3f32bfbc78 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle)
>   	return 0;
>   }
>   
> +/**
> + * 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;
> +}
> +
>   /**
>    * ice_read_sma_ctrl_e810t
>    * @hw: pointer to the hw struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 4e3c1382c477..3768e7a01920 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8 port);
>   int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
>   
>   /* E810 family functions */
> +bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
>   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);

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Keller, Jacob E]

> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Sent: Wednesday, August 16, 2023 3:54 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-wired-
> lan@lists.osuosl.org>
> Cc: Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before
> enabling ICE_F_SMA_CTRL
> 
> On 8/16/23 00:35, Jacob Keller wrote:
> > Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
> > E810-T based devices. In some cases, the SMA control access is not
> > available in the netlist firmware component. In such a case, the driver
> > will fail to setup the SMA pins. When this happens, the driver prints
> > "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
> > pin configuration support.
> >
> > This results in failure to use even the fixed pin capabilities of standard
> > E810 devices, resulting in reduced functionality.
> >
> > To avoid this, check the netlist for the SMA control module before enabling
> > the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
> > feature will not be enabled. In this case, the driver flow for enabling
> > periodic outputs and external timestamps will fall back to the standard
> > fixed pin configuration.
> >
> > This allows supporting the software defined pins on a wider array of
> > platforms.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Overall it's a nice series!
> 
> > ---
> >   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
> >   drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
> >   drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
> >   drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
> >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
> >   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
> >   6 files changed, 72 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > index 90616750e779..82c4daf0a825 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > @@ -1367,6 +1367,7 @@ 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_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)
> > @@ -1403,8 +1404,9 @@ 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_C827	0x31
> > +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575
> 	0x21
> > +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827
> 	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 2652e4f5c4a2..9eeda3f5aa75 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
> >   	return false;
> >   }
> >
> > +#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 0 is returned, -ENOENT otherwise. If
> > + * node_handle provided, it would be set to found node handle.
> > + */
> > +int
> > +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;
> > +	u16 rec_node_handle;
> 
> I see that you are using separate variable to 'do not touch'
> @node_handle param if it does not need to be updated.
> But perhaps you could consider to just pass @node_handle in place of
> @rec_node_handle below, and have less code?
> I do not see any non-null usage of the field anyway.
> 
> (rationale: our code is so self-similar that I needed to check wheater
> you are basing-of recent changes by Jan&Karol or re-doing them ;)
> answer: we are fine here :)).

I can look at that. (I mostly took this from the implementation as-is in the sourceforge driver...)

> 
> > +	u8 idx;
> > +
> > +	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
> > +		int status;
> > +
> > +		memset(&cmd, 0, sizeof(cmd));
> > +
> > +		cmd.addr.topo_params.node_type_ctx =
> > +			(node_type_ctx <<
> ICE_AQC_LINK_TOPO_NODE_TYPE_S);
> 
> I would FIELD_PREP() here
> 
> and perhaps convert @cmd scope to inside the loop, that would enable = {
> .addr.topo_params = { ... } } declaration


I can look into that.

> 
> > +		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 0;
> > +		}
> > +	}
> > +
> > +	return -ENOENT;
> > +}
> > +
> >   /**
> >    * ice_aq_list_caps - query function/device capabilities
> >    * @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 2250a9c5f61e..f7178a5686a5 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> > @@ -94,6 +94,9 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool
> qual_mods, u8 report_mode,
> >   		    struct ice_sq_cd *cd);
> >   bool ice_is_pf_c827(struct ice_hw *hw);
> >   int
> > +ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8
> node_part_number,
> > +		      u16 *node_handle);
> > +int
> >   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);
> >   int
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index f29ff54383b5..4ac8998cb964 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > @@ -3989,8 +3989,9 @@ void ice_init_feature_support(struct ice_pf *pf)
> >   		/* If we don't own the timer - don't enable other caps */
> >   		if (!ice_pf_src_tmr_owned(pf))
> >   			break;
> > -		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_e810t(&pf->hw)) {
> >   			if (ice_gnss_is_gps_present(&pf->hw))
> >   				ice_set_feature_support(pf, ICE_F_GNSS);
> >   		}
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > index fd19afaf9c85..bd3f32bfbc78 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > @@ -3018,6 +3018,22 @@ ice_get_pca9575_handle(struct ice_hw *hw, u16
> *pca9575_handle)
> >   	return 0;
> >   }
> >
> > +/**
> > + * 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;
> > +}
> > +
> >   /**
> >    * ice_read_sma_ctrl_e810t
> >    * @hw: pointer to the hw struct
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > index 4e3c1382c477..3768e7a01920 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > @@ -195,6 +195,7 @@ int ice_phy_cfg_tx_offset_e822(struct ice_hw *hw, u8
> port);
> >   int ice_phy_cfg_rx_offset_e822(struct ice_hw *hw, u8 port);
> >
> >   /* E810 family functions */
> > +bool ice_is_clock_mux_present_e810t(struct ice_hw *hw);
> >   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);

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Jacob Keller]

On 8/16/2023 3:54 AM, Przemek Kitszel wrote:
> On 8/16/23 00:35, Jacob Keller wrote:
>> Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
>> E810-T based devices. In some cases, the SMA control access is not
>> available in the netlist firmware component. In such a case, the driver
>> will fail to setup the SMA pins. When this happens, the driver prints
>> "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
>> pin configuration support.
>>
>> This results in failure to use even the fixed pin capabilities of standard
>> E810 devices, resulting in reduced functionality.
>>
>> To avoid this, check the netlist for the SMA control module before enabling
>> the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
>> feature will not be enabled. In this case, the driver flow for enabling
>> periodic outputs and external timestamps will fall back to the standard
>> fixed pin configuration.
>>
>> This allows supporting the software defined pins on a wider array of
>> platforms.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Overall it's a nice series!
> 
>> ---
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
>>   drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
>>   drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
>>   6 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> index 90616750e779..82c4daf0a825 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> @@ -1367,6 +1367,7 @@ 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_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)
>> @@ -1403,8 +1404,9 @@ 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_C827	0x31
>> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
>> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
>>   	return false;
>>   }
>>   
>> +#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 0 is returned, -ENOENT otherwise. If
>> + * node_handle provided, it would be set to found node handle.
>> + */
>> +int
>> +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;
>> +	u16 rec_node_handle;
> 
> I see that you are using separate variable to 'do not touch' 
> @node_handle param if it does not need to be updated.
> But perhaps you could consider to just pass @node_handle in place of 
> @rec_node_handle below, and have less code?
> I do not see any non-null usage of the field anyway.
> 
> (rationale: our code is so self-similar that I needed to check wheater 
> you are basing-of recent changes by Jan&Karol or re-doing them ;)
> answer: we are fine here :)).
> 
>> +	u8 idx;
>> +
>> +	for (idx = 0; idx < MAX_NETLIST_SIZE; idx++) {
>> +		int status;
>> +
>> +		memset(&cmd, 0, sizeof(cmd));
>> +
>> +		cmd.addr.topo_params.node_type_ctx =
>> +			(node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S);
> 
> I would FIELD_PREP() here
> 
> and perhaps convert @cmd scope to inside the loop, that would enable = { 
> .addr.topo_params = { ... } } declaration
> 

I couldn't find a nice way to make the .addr.topo_params = { ... } to
look ok, so I opted against it. I did reduce variable scope though.

Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: check the netlist before enabling ICE_F_SMA_CTRL

[Jacob Keller]

On 8/16/2023 3:54 AM, Przemek Kitszel wrote:
> On 8/16/23 00:35, Jacob Keller wrote:
>> Currently, the ice driver unconditionally enables ICE_F_SMA_CTRL for all
>> E810-T based devices. In some cases, the SMA control access is not
>> available in the netlist firmware component. In such a case, the driver
>> will fail to setup the SMA pins. When this happens, the driver prints
>> "Failed to configure E810-T SMA pin control" and forcibly disables all PTP
>> pin configuration support.
>>
>> This results in failure to use even the fixed pin capabilities of standard
>> E810 devices, resulting in reduced functionality.
>>
>> To avoid this, check the netlist for the SMA control module before enabling
>> the ICE_F_SMA_CTRL feature. If the netlist lacks this module, then the
>> feature will not be enabled. In this case, the driver flow for enabling
>> periodic outputs and external timestamps will fall back to the standard
>> fixed pin configuration.
>>
>> This allows supporting the software defined pins on a wider array of
>> platforms.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Overall it's a nice series!
> 
>> ---
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  6 ++-
>>   drivers/net/ethernet/intel/ice/ice_common.c   | 46 +++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_common.h   |  3 ++
>>   drivers/net/ethernet/intel/ice/ice_lib.c      |  3 +-
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 16 +++++++
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |  1 +
>>   6 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> index 90616750e779..82c4daf0a825 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> @@ -1367,6 +1367,7 @@ 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_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)
>> @@ -1403,8 +1404,9 @@ 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_C827	0x31
>> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575			0x21
>> +#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827			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 2652e4f5c4a2..9eeda3f5aa75 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -2503,6 +2503,52 @@ bool ice_is_pf_c827(struct ice_hw *hw)
>>   	return false;
>>   }
>>   
>> +#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 0 is returned, -ENOENT otherwise. If
>> + * node_handle provided, it would be set to found node handle.
>> + */
>> +int
>> +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;
>> +	u16 rec_node_handle;
> 
> I see that you are using separate variable to 'do not touch' 
> @node_handle param if it does not need to be updated.
> But perhaps you could consider to just pass @node_handle in place of 
> @rec_node_handle below, and have less code?
> I do not see any non-null usage of the field anyway.
> 
> (rationale: our code is so self-similar that I needed to check wheater 
> you are basing-of recent changes by Jan&Karol or re-doing them ;)
> answer: we are fine here :)).
> 

There will be users of this function soon which want to get the node
handle out, so I kept that functionality, but I did get rid of the extra
variable. No user should ever care about the node_handle being modified
on error. I'll note it in the function comment though.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan