[PATCH iwl-next v1 0/3] E825C timesync dual NAC support

[PATCH iwl-next v1 0/3] E825C timesync dual NAC support

[Grzegorz Nitka]

This patch series adds full support for timesync operations for E8225C
devices which are configured in so called 2xNAC mode (Network
Acceleration Complex). 2xNAC mode is the mode in which IO die
is housing two complexes and each of them has its own PHY connected
to it. The complex which controls time transmitter is referred as
primary complex.

The series solves known configuration issues in dual config mode:
- side-band queue (SBQ) addressing when configuring the ports on the PHY
  on secondary NAC
- access to timesync config from the second NAC as only one PF in
  primary NAC controls time transmitter clock
Karol Kolacinski (3):
  ice: remove SW side band access workaround for E825
  ice: refactor ice_sbq_msg_dev enum
  ice: enable timesync operation on 2xNAC E825 devices

 drivers/net/ethernet/intel/ice/ice.h         | 60 +++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.c  |  8 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 49 +++++++++---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 82 ++++++++++----------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h  |  5 --
 drivers/net/ethernet/intel/ice/ice_sbq_cmd.h | 11 +--
 drivers/net/ethernet/intel/ice/ice_type.h    |  1 +
 7 files changed, 149 insertions(+), 67 deletions(-)


base-commit: 692375ca2a4e6916ddc2ef0d73faa37c7a93cd1a
-- 
2.39.3

[PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

[Grzegorz Nitka]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Due to the bug in FW/NVM autoload mechanism (wrong default
SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
clients was disabled by default.
As the workaround solution, the register value was overwritten by the
driver at the probe or reset handling.
Remove workaround as it's not needed anymore. The fix in autoload
procedure has been provided with NVM 3.80 version.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 89bb8461284a..a5df081ffc19 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2630,25 +2630,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	return 0;
 }
 
-/**
- * ice_sb_access_ena_eth56g - Enable SB devices (PHY and others) access
- * @hw: pointer to HW struct
- * @enable: Enable or disable access
- *
- * Enable sideband devices (PHY and others) access.
- */
-static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
-{
-	u32 val = rd32(hw, PF_SB_REM_DEV_CTL);
-
-	if (enable)
-		val |= BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1);
-	else
-		val &= ~(BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1));
-
-	wr32(hw, PF_SB_REM_DEV_CTL, val);
-}
-
 /**
  * ice_ptp_init_phc_e825 - Perform E825 specific PHC initialization
  * @hw: pointer to HW struct
@@ -2659,8 +2640,6 @@ static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
  */
 static int ice_ptp_init_phc_e825(struct ice_hw *hw)
 {
-	ice_sb_access_ena_eth56g(hw, true);
-
 	/* Initialize the Clock Generation Unit */
 	return ice_init_cgu_e82x(hw);
 }
@@ -2747,8 +2726,6 @@ static void ice_ptp_init_phy_e825(struct ice_hw *hw)
 	params->num_phys = 2;
 	ptp->ports_per_phy = 4;
 	ptp->num_lports = params->num_phys * ptp->ports_per_phy;
-
-	ice_sb_access_ena_eth56g(hw, true);
 }
 
 /* E822 family functions
-- 
2.39.3

[PATCH iwl-next v1 2/3] ice: refactor ice_sbq_msg_dev enum

[Grzegorz Nitka]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Rename ice_sbq_msg_dev to ice_sbq_dev_id to reflect the meaning of this
type more precisely. This enum type describes RDA (Remote Device Access)
client ids, accessible over SB (Side Band) interface.
Rename enum elements to make a driver namespace more cleaner and
consistent with other definitions within SB
Remove unused 'rmn_x' entries, specific to unsupported E824 device.
Adjust clients '2' and '13' names (phy_0 and phy_0_peer respectively) to
be compliant with EAS doc. According to the specification, regardless of
the complex entity (single or dual), when accessing its own ports,
they're accessed always as 'phy_0' client. And referred as 'phy_0_peer'
when handling ports conneced to the other complex.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c  |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 20 ++++++++++----------
 drivers/net/ethernet/intel/ice/ice_sbq_cmd.h | 11 ++++-------
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index f2e51bacecf8..ed7ef8608cbb 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -3433,7 +3433,7 @@ int ice_aq_get_fec_stats(struct ice_hw *hw, u16 pcs_quad, u16 pcs_port,
 	msg.msg_addr_low = lower_16_bits(reg_offset);
 	msg.msg_addr_high = receiver_id;
 	msg.opcode = ice_sbq_msg_rd;
-	msg.dest_dev = rmn_0;
+	msg.dest_dev = ice_sbq_dev_phy_0;
 
 	err = ice_sbq_rw_reg(hw, &msg, flag);
 	if (err)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index a5df081ffc19..eb1893dd8979 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -240,7 +240,7 @@ static int ice_read_cgu_reg_e82x(struct ice_hw *hw, u32 addr, u32 *val)
 {
 	struct ice_sbq_msg_input cgu_msg = {
 		.opcode = ice_sbq_msg_rd,
-		.dest_dev = cgu,
+		.dest_dev = ice_sbq_dev_cgu,
 		.msg_addr_low = addr
 	};
 	int err;
@@ -272,7 +272,7 @@ static int ice_write_cgu_reg_e82x(struct ice_hw *hw, u32 addr, u32 val)
 {
 	struct ice_sbq_msg_input cgu_msg = {
 		.opcode = ice_sbq_msg_wr,
-		.dest_dev = cgu,
+		.dest_dev = ice_sbq_dev_cgu,
 		.msg_addr_low = addr,
 		.data = val
 	};
@@ -919,16 +919,16 @@ static void ice_ptp_cfg_sync_delay(const struct ice_hw *hw, u32 delay)
  *
  * Return: destination sideband queue PHY device.
  */
-static enum ice_sbq_msg_dev ice_ptp_get_dest_dev_e825(struct ice_hw *hw,
-						      u8 port)
+static enum ice_sbq_dev_id ice_ptp_get_dest_dev_e825(struct ice_hw *hw,
+						     u8 port)
 {
 	/* On a single complex E825, PHY 0 is always destination device phy_0
 	 * and PHY 1 is phy_0_peer.
 	 */
 	if (port >= hw->ptp.ports_per_phy)
-		return eth56g_phy_1;
+		return ice_sbq_dev_phy_0_peer;
 	else
-		return eth56g_phy_0;
+		return ice_sbq_dev_phy_0;
 }
 
 /**
@@ -2758,7 +2758,7 @@ static void ice_fill_phy_msg_e82x(struct ice_hw *hw,
 		msg->msg_addr_high = P_Q1_H(P_4_BASE + offset, phy_port);
 	}
 
-	msg->dest_dev = rmn_0;
+	msg->dest_dev = ice_sbq_dev_phy_0;
 }
 
 /**
@@ -3081,7 +3081,7 @@ static int ice_fill_quad_msg_e82x(struct ice_hw *hw,
 	if (quad >= ICE_GET_QUAD_NUM(hw->ptp.num_lports))
 		return -EINVAL;
 
-	msg->dest_dev = rmn_0;
+	msg->dest_dev = ice_sbq_dev_phy_0;
 
 	if (!(quad % ICE_GET_QUAD_NUM(hw->ptp.ports_per_phy)))
 		addr = Q_0_BASE + offset;
@@ -4800,7 +4800,7 @@ static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
 	msg.msg_addr_low = lower_16_bits(addr);
 	msg.msg_addr_high = upper_16_bits(addr);
 	msg.opcode = ice_sbq_msg_rd;
-	msg.dest_dev = rmn_0;
+	msg.dest_dev = ice_sbq_dev_phy_0;
 
 	err = ice_sbq_rw_reg(hw, &msg, ICE_AQ_FLAG_RD);
 	if (err) {
@@ -4830,7 +4830,7 @@ static int ice_write_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 val)
 	msg.msg_addr_low = lower_16_bits(addr);
 	msg.msg_addr_high = upper_16_bits(addr);
 	msg.opcode = ice_sbq_msg_wr;
-	msg.dest_dev = rmn_0;
+	msg.dest_dev = ice_sbq_dev_phy_0;
 	msg.data = val;
 
 	err = ice_sbq_rw_reg(hw, &msg, ICE_AQ_FLAG_RD);
diff --git a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
index 3b0054faf70c..183dd5457d6a 100644
--- a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
@@ -46,13 +46,10 @@ struct ice_sbq_evt_desc {
 	u8 data[24];
 };
 
-enum ice_sbq_msg_dev {
-	eth56g_phy_0	= 0x02,
-	rmn_0		= 0x02,
-	rmn_1		= 0x03,
-	rmn_2		= 0x04,
-	cgu		= 0x06,
-	eth56g_phy_1	= 0x0D,
+enum ice_sbq_dev_id {
+	ice_sbq_dev_phy_0	= 0x02,
+	ice_sbq_dev_cgu		= 0x06,
+	ice_sbq_dev_phy_0_peer	= 0x0D,
 };
 
 enum ice_sbq_msg_opcode {
-- 
2.39.3

[PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

[Grzegorz Nitka]

From: Karol Kolacinski <karol.kolacinski@intel.com>

According to the E825C specification, SBQ address for ports on a single
complex is device 2 for PHY 0 and device 13 for PHY1.
For accessing ports on a dual complex E825C (so called 2xNAC mode),
the driver should use destination device 2 (referred as phy_0) for
the current complex PHY and device 13 (referred as phy_0_peer) for
peer complex PHY.

Differentiate SBQ destination device by checking if current PF port
number is on the same PHY as target port number.

Adjust 'ice_get_lane_number' function to provide unique port number for
ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
ports). Cache this value in ice_hw struct.

Introduce ice_get_primary_hw wrapper to get access to timesync register
not available from second NAC.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h        | 60 ++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.c |  6 ++-
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 49 ++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++++++++++---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  5 --
 drivers/net/ethernet/intel/ice/ice_type.h   |  1 +
 6 files changed, 134 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 53b990e2e850..d80ab48402f1 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -193,8 +193,6 @@
 
 #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
 
-#define ice_pf_src_tmr_owned(pf) ((pf)->hw.func_caps.ts_func_info.src_tmr_owned)
-
 enum ice_feature {
 	ICE_F_DSCP,
 	ICE_F_PHY_RCLK,
@@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct ice_pf *pf)
 }
 
 extern const struct xdp_metadata_ops ice_xdp_md_ops;
+
+/**
+ * ice_is_dual - Check if given config is multi-NAC
+ * @hw: pointer to HW structure
+ *
+ * Return: true if the device is running in mutli-NAC (Network
+ * Acceleration Complex) configuration variant, false otherwise
+ * (always false for non-E825 devices).
+ */
+static inline bool ice_is_dual(struct ice_hw *hw)
+{
+	return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
+	       (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
+}
+
+/**
+ * ice_is_primary - Check if given device belongs to the primary complex
+ * @hw: pointer to HW structure
+ *
+ * Check if given PF/HW is running on primary complex in multi-NAC
+ * configuration.
+ *
+ * Return: true if the device is dual, false otherwise (always true
+ * for non-E825 devices).
+ */
+static inline bool ice_is_primary(struct ice_hw *hw)
+{
+	return hw->mac_type != ICE_MAC_GENERIC_3K_E825 ||
+	       !ice_is_dual(hw) ||
+	       (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_PRIMARY_M);
+}
+
+/**
+ * ice_pf_src_tmr_owned - Check if a primary timer is owned by PF
+ * @hw: pointer to HW structure
+ *
+ * Return: true if PF owns primary timer, false otherwise.
+ */
+static inline bool ice_pf_src_tmr_owned(struct ice_pf *pf)
+{
+	return pf->hw.func_caps.ts_func_info.src_tmr_owned &&
+	       ice_is_primary(&pf->hw);
+}
+
+/**
+ * ice_get_primary_hw - Get pointer to primary ice_hw structure
+ * @pf: pointer to PF structure
+ *
+ * Return: A pointer to ice_hw structure with access to timesync
+ * register space.
+ */
+static inline struct ice_hw *ice_get_primary_hw(struct ice_pf *pf)
+{
+	if (!pf->adapter->ctrl_pf)
+		return &pf->hw;
+	else
+		return &pf->adapter->ctrl_pf->hw;
+}
 #endif /* _ICE_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index ed7ef8608cbb..4ff4c99d0872 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
 		}
 	}
 
+	hw->lane_num = ice_get_phy_lane_number(hw);
+
 	return 0;
 err_unroll_fltr_mgmt_struct:
 	ice_cleanup_fltr_mgmt_struct(hw);
@@ -4081,10 +4083,12 @@ int ice_get_phy_lane_number(struct ice_hw *hw)
 			continue;
 
 		if (hw->pf_id == lport) {
+			if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
+			    ice_is_dual(hw) && !ice_is_primary(hw))
+				lane += ICE_PORTS_PER_QUAD;
 			kfree(options);
 			return lane;
 		}
-
 		lport++;
 	}
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a7aa6d5fb775..81012024c14f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -208,6 +208,9 @@ u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 	u32 hi, lo, lo2;
 	u8 tmr_idx;
 
+	if (!ice_is_primary(hw))
+		hw = ice_get_primary_hw(pf);
+
 	tmr_idx = ice_get_ptp_src_clock_index(hw);
 	guard(spinlock)(&pf->adapter->ptp_gltsyn_time_lock);
 	/* Read the system timestamp pre PHC read */
@@ -2807,6 +2810,32 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
 				   msecs_to_jiffies(err ? 10 : 500));
 }
 
+/**
+ * ice_ptp_prepare_rebuild_sec - Prepare second NAC for PTP reset or rebuild
+ * @pf: Board private structure
+ * @rebuild: rebuild if true, prepare if false
+ * @reset_type: the reset type being performed
+ */
+static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
+					enum ice_reset_req reset_type)
+{
+	struct list_head *entry;
+
+	list_for_each(entry, &pf->adapter->ports.ports) {
+		struct ice_ptp_port *port = list_entry(entry,
+						       struct ice_ptp_port,
+						       list_node);
+		struct ice_pf *peer_pf = ptp_port_to_pf(port);
+
+		if (!ice_is_primary(&peer_pf->hw)) {
+			if (rebuild)
+				ice_ptp_rebuild(peer_pf, reset_type);
+			else
+				ice_ptp_prepare_for_reset(peer_pf, reset_type);
+		}
+	}
+}
+
 /**
  * ice_ptp_prepare_for_reset - Prepare PTP for reset
  * @pf: Board private structure
@@ -2815,6 +2844,7 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
 void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 {
 	struct ice_ptp *ptp = &pf->ptp;
+	struct ice_hw *hw = &pf->hw;
 	u8 src_tmr;
 
 	if (ptp->state != ICE_PTP_READY)
@@ -2830,6 +2860,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (reset_type == ICE_RESET_PFR)
 		return;
 
+	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
+		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
+
 	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
 
 	/* Disable periodic outputs */
@@ -2951,13 +2984,6 @@ void ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
 	dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);
 }
 
-static bool ice_is_primary(struct ice_hw *hw)
-{
-	return hw->mac_type == ICE_MAC_GENERIC_3K_E825 && ice_is_dual(hw) ?
-		       !!(hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_PRIMARY_M) :
-		       true;
-}
-
 static int ice_ptp_setup_adapter(struct ice_pf *pf)
 {
 	if (!ice_pf_src_tmr_owned(pf) || !ice_is_primary(&pf->hw))
@@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
 {
 	struct ice_ptp *ptp = &pf->ptp;
 	struct ice_hw *hw = &pf->hw;
-	int lane_num, err;
+	int err;
 
 	ptp->state = ICE_PTP_INITIALIZING;
 
-	lane_num = ice_get_phy_lane_number(hw);
-	if (lane_num < 0) {
-		err = lane_num;
+	if (hw->lane_num < 0) {
+		err = hw->lane_num;
 		goto err_exit;
 	}
+	ptp->port.port_num = hw->lane_num;
 
-	ptp->port.port_num = (u8)lane_num;
 	ice_ptp_init_hw(hw);
 
 	ice_ptp_init_tx_interrupt_mode(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index eb1893dd8979..ccac84eb34c9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -874,8 +874,12 @@ static u32 ice_ptp_tmr_cmd_to_port_reg(struct ice_hw *hw,
  */
 void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
 {
+	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
 	u32 cmd_val = ice_ptp_tmr_cmd_to_src_reg(hw, cmd);
 
+	if (!ice_is_primary(hw))
+		hw = ice_get_primary_hw(pf);
+
 	wr32(hw, GLTSYN_CMD, cmd_val);
 }
 
@@ -891,6 +895,9 @@ static void ice_ptp_exec_tmr_cmd(struct ice_hw *hw)
 {
 	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
 
+	if (!ice_is_primary(hw))
+		hw = ice_get_primary_hw(pf);
+
 	guard(spinlock)(&pf->adapter->ptp_gltsyn_time_lock);
 	wr32(hw, GLTSYN_CMD_SYNC, SYNC_EXEC_CMD);
 	ice_flush(hw);
@@ -922,10 +929,18 @@ static void ice_ptp_cfg_sync_delay(const struct ice_hw *hw, u32 delay)
 static enum ice_sbq_dev_id ice_ptp_get_dest_dev_e825(struct ice_hw *hw,
 						     u8 port)
 {
-	/* On a single complex E825, PHY 0 is always destination device phy_0
+	u8 curr_phy, tgt_phy;
+
+	tgt_phy = port >= hw->ptp.ports_per_phy;
+	curr_phy = hw->lane_num >= hw->ptp.ports_per_phy;
+	/* In the driver, lanes 4..7 are in fact 0..3 on a second PHY.
+	 * On a single complex E825C, PHY 0 is always destination device phy_0
 	 * and PHY 1 is phy_0_peer.
+	 * On dual complex E825C, device phy_0 points to PHY on a current
+	 * complex and phy_0_peer to PHY on a different complex.
 	 */
-	if (port >= hw->ptp.ports_per_phy)
+	if ((!ice_is_dual(hw) && tgt_phy == 1) ||
+	    (ice_is_dual(hw) && tgt_phy != curr_phy))
 		return ice_sbq_dev_phy_0_peer;
 	else
 		return ice_sbq_dev_phy_0;
@@ -2417,6 +2432,7 @@ int ice_phy_cfg_intr_eth56g(struct ice_hw *hw, u8 port, bool ena, u8 threshold)
 static int ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
 					    u64 *phy_time, u64 *phc_time)
 {
+	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
 	u64 tx_time, rx_time;
 	u32 zo, lo;
 	u8 tmr_idx;
@@ -2436,8 +2452,13 @@ static int ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
 	ice_ptp_exec_tmr_cmd(hw);
 
 	/* Read the captured PHC time from the shadow time registers */
-	zo = rd32(hw, GLTSYN_SHTIME_0(tmr_idx));
-	lo = rd32(hw, GLTSYN_SHTIME_L(tmr_idx));
+	if (ice_is_primary(hw)) {
+		zo = rd32(hw, GLTSYN_SHTIME_0(tmr_idx));
+		lo = rd32(hw, GLTSYN_SHTIME_L(tmr_idx));
+	} else {
+		zo = rd32(ice_get_primary_hw(pf), GLTSYN_SHTIME_0(tmr_idx));
+		lo = rd32(ice_get_primary_hw(pf), GLTSYN_SHTIME_L(tmr_idx));
+	}
 	*phc_time = (u64)lo << 32 | zo;
 
 	/* Read the captured PHY time from the PHY shadow registers */
@@ -2574,6 +2595,7 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
  */
 int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 {
+	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
 	u32 lo, hi;
 	u64 incval;
 	u8 tmr_idx;
@@ -2599,8 +2621,13 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	if (err)
 		return err;
 
-	lo = rd32(hw, GLTSYN_INCVAL_L(tmr_idx));
-	hi = rd32(hw, GLTSYN_INCVAL_H(tmr_idx));
+	if (ice_is_primary(hw)) {
+		lo = rd32(hw, GLTSYN_INCVAL_L(tmr_idx));
+		hi = rd32(hw, GLTSYN_INCVAL_H(tmr_idx));
+	} else {
+		lo = rd32(ice_get_primary_hw(pf), GLTSYN_INCVAL_L(tmr_idx));
+		hi = rd32(ice_get_primary_hw(pf), GLTSYN_INCVAL_H(tmr_idx));
+	}
 	incval = (u64)hi << 32 | lo;
 
 	err = ice_write_40b_ptp_reg_eth56g(hw, port, PHY_REG_TIMETUS_L, incval);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index e5925ccc2613..83f20fa7ace7 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -444,11 +444,6 @@ static inline u64 ice_get_base_incval(struct ice_hw *hw)
 	}
 }
 
-static inline bool ice_is_dual(struct ice_hw *hw)
-{
-	return !!(hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
-}
-
 #define PFTSYN_SEM_BYTES	4
 
 #define ICE_PTP_CLOCK_INDEX_0	0x00
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 0aab21113cc4..04123dfd20d2 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -970,6 +970,7 @@ struct ice_hw {
 	u8 intrl_gran;
 
 	struct ice_ptp_hw ptp;
+	u8 lane_num;
 
 	/* Active package version (currently active) */
 	struct ice_pkg_ver active_pkg_ver;
-- 
2.39.3

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

[Paul Menzel]

Dear Grzegorz, dear Karol,


Thank you for your patch.

Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Due to the bug in FW/NVM autoload mechanism (wrong default
> SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> clients was disabled by default.

I’d add a blank line between the paragraphs.

> As the workaround solution, the register value was overwritten by the
> driver at the probe or reset handling.
> Remove workaround as it's not needed anymore. The fix in autoload
> procedure has been provided with NVM 3.80 version.

Is this compatible with Linux’ no regression policy? People might only 
update the Linux kernel and not the firmware.

How did you test this, and how can others test this?

> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
>   1 file changed, 23 deletions(-)


Kind regards,

Paul


> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 89bb8461284a..a5df081ffc19 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -2630,25 +2630,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>   	return 0;
>   }
>   
> -/**
> - * ice_sb_access_ena_eth56g - Enable SB devices (PHY and others) access
> - * @hw: pointer to HW struct
> - * @enable: Enable or disable access
> - *
> - * Enable sideband devices (PHY and others) access.
> - */
> -static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
> -{
> -	u32 val = rd32(hw, PF_SB_REM_DEV_CTL);
> -
> -	if (enable)
> -		val |= BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1);
> -	else
> -		val &= ~(BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1));
> -
> -	wr32(hw, PF_SB_REM_DEV_CTL, val);
> -}
> -
>   /**
>    * ice_ptp_init_phc_e825 - Perform E825 specific PHC initialization
>    * @hw: pointer to HW struct
> @@ -2659,8 +2640,6 @@ static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
>    */
>   static int ice_ptp_init_phc_e825(struct ice_hw *hw)
>   {
> -	ice_sb_access_ena_eth56g(hw, true);
> -
>   	/* Initialize the Clock Generation Unit */
>   	return ice_init_cgu_e82x(hw);
>   }
> @@ -2747,8 +2726,6 @@ static void ice_ptp_init_phy_e825(struct ice_hw *hw)
>   	params->num_phys = 2;
>   	ptp->ports_per_phy = 4;
>   	ptp->num_lports = params->num_phys * ptp->ports_per_phy;
> -
> -	ice_sb_access_ena_eth56g(hw, true);
>   }
>   
>   /* E822 family functions

Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

[Simon Horman]

On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> According to the E825C specification, SBQ address for ports on a single
> complex is device 2 for PHY 0 and device 13 for PHY1.
> For accessing ports on a dual complex E825C (so called 2xNAC mode),
> the driver should use destination device 2 (referred as phy_0) for
> the current complex PHY and device 13 (referred as phy_0_peer) for
> peer complex PHY.
> 
> Differentiate SBQ destination device by checking if current PF port
> number is on the same PHY as target port number.
> 
> Adjust 'ice_get_lane_number' function to provide unique port number for
> ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
> ports). Cache this value in ice_hw struct.
> 
> Introduce ice_get_primary_hw wrapper to get access to timesync register
> not available from second NAC.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice.h        | 60 ++++++++++++++++++++-
>  drivers/net/ethernet/intel/ice/ice_common.c |  6 ++-
>  drivers/net/ethernet/intel/ice/ice_ptp.c    | 49 ++++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++++++++++---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  5 --
>  drivers/net/ethernet/intel/ice/ice_type.h   |  1 +
>  6 files changed, 134 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 53b990e2e850..d80ab48402f1 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -193,8 +193,6 @@
>  
>  #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
>  
> -#define ice_pf_src_tmr_owned(pf) ((pf)->hw.func_caps.ts_func_info.src_tmr_owned)
> -
>  enum ice_feature {
>  	ICE_F_DSCP,
>  	ICE_F_PHY_RCLK,
> @@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct ice_pf *pf)
>  }
>  
>  extern const struct xdp_metadata_ops ice_xdp_md_ops;
> +
> +/**
> + * ice_is_dual - Check if given config is multi-NAC
> + * @hw: pointer to HW structure
> + *
> + * Return: true if the device is running in mutli-NAC (Network
> + * Acceleration Complex) configuration variant, false otherwise
> + * (always false for non-E825 devices).
> + */
> +static inline bool ice_is_dual(struct ice_hw *hw)
> +{
> +	return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> +	       (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
> +}

Thanks for the complete Kernel doc, and not adding unnecessary () around
the value returned. Overall these helpers seem nice and clean to me.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index ed7ef8608cbb..4ff4c99d0872 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
>  		}
>  	}
>  
> +	hw->lane_num = ice_get_phy_lane_number(hw);
> +

Unfortunately there seems to be a bit of a problem here:

The type of hw->lane number is u8.
But ice_get_phy_lane_number returns an int,
which may be a negative error value...

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c

...

> @@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
>  {
>  	struct ice_ptp *ptp = &pf->ptp;
>  	struct ice_hw *hw = &pf->hw;
> -	int lane_num, err;
> +	int err;
>  
>  	ptp->state = ICE_PTP_INITIALIZING;
>  
> -	lane_num = ice_get_phy_lane_number(hw);
> -	if (lane_num < 0) {
> -		err = lane_num;
> +	if (hw->lane_num < 0) {

... which is checked here.

But because hw->lane_num is unsigned, this condition is always false.

FWIIW, I think it is nice that that hw->lane is a u8.
But error handling seems broken here.

> +		err = hw->lane_num;
>  		goto err_exit;
>  	}
> +	ptp->port.port_num = hw->lane_num;
>  
> -	ptp->port.port_num = (u8)lane_num;
>  	ice_ptp_init_hw(hw);
>  
>  	ice_ptp_init_tx_interrupt_mode(pf);

...

Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

[Simon Horman]

On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:

...

> +/**
> + * ice_pf_src_tmr_owned - Check if a primary timer is owned by PF
> + * @hw: pointer to HW structure

Sorry, I hit send for my previous email a little to early.

This should document @pf rather than @hw.

> + *
> + * Return: true if PF owns primary timer, false otherwise.
> + */
> +static inline bool ice_pf_src_tmr_owned(struct ice_pf *pf)
> +{
> +	return pf->hw.func_caps.ts_func_info.src_tmr_owned &&
> +	       ice_is_primary(&pf->hw);
> +}

...

Re: [PATCH iwl-next v1 2/3] ice: refactor ice_sbq_msg_dev enum

[Simon Horman]

On Fri, Feb 21, 2025 at 01:31:22PM +0100, Grzegorz Nitka wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Rename ice_sbq_msg_dev to ice_sbq_dev_id to reflect the meaning of this
> type more precisely. This enum type describes RDA (Remote Device Access)
> client ids, accessible over SB (Side Band) interface.
> Rename enum elements to make a driver namespace more cleaner and
> consistent with other definitions within SB
> Remove unused 'rmn_x' entries, specific to unsupported E824 device.
> Adjust clients '2' and '13' names (phy_0 and phy_0_peer respectively) to
> be compliant with EAS doc. According to the specification, regardless of
> the complex entity (single or dual), when accessing its own ports,
> they're accessed always as 'phy_0' client. And referred as 'phy_0_peer'
> when handling ports conneced to the other complex.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

[Nitka, Grzegorz]

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Friday, February 21, 2025 10:16 PM
> To: Nitka, Grzegorz <grzegorz.nitka@intel.com>; Kolacinski, Karol
> <karol.kolacinski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> band access workaround for E825
> 
> Dear Grzegorz, dear Karol,
> 
> 
> Thank you for your patch.
> 
> Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> > From: Karol Kolacinski <karol.kolacinski@intel.com>
> >
> > Due to the bug in FW/NVM autoload mechanism (wrong default
> > SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> > clients was disabled by default.
> 
> I’d add a blank line between the paragraphs.
> 
> > As the workaround solution, the register value was overwritten by the
> > driver at the probe or reset handling.
> > Remove workaround as it's not needed anymore. The fix in autoload
> > procedure has been provided with NVM 3.80 version.
> 
> Is this compatible with Linux’ no regression policy? People might only
> update the Linux kernel and not the firmware.
> 
> How did you test this, and how can others test this?
> 
> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
> >   1 file changed, 23 deletions(-)
> 
> 
> Kind regards,
> 
> Paul
> 
> 

Dear Paul, first of all thank you for your review and apologize for late
response (simply, I was out previous week).

Removing that workaround was a conscious decision.
Rationale for that is, that the 'problematic' workaround was provided
in very early product development stage (~ year ago).  Now, especially
when E825-C product was just officially announced, the customer shall
use official, approved SW ingredients.

Here are more details on this:
- introduction to E825-C devices was provided in kernel 6.6, to allow
  selected customers for early E825-C enablement. At that time E825-C
  product family was in early phase (kind of Alpha), and one of the
  consequences, from today's perspective,  is that it included 'unwanted'
  workarounds like this.

- this change applies to E825-C products only, which is SoC product, not
  a regular NIC card.  What I'd like to emphasize here, it requires even more
  customer support from Intel company in terms of providing matching
  SW stack (like BIOS, firmware, drivers etc.).
  
I see two possibilities now:
1) if the regression policy you mentioned is inviolable, keep the workaround
   in the kernel code like it is today. Maybe we could add NVM version checker
   and apply register updates for older NVMs only.
   But, in my opinion, it would lead to keeping a dead code. There shouldn't be
   official FW (I hope I won't regret these words) on the market which requires
   this workaround.
  
2) remove the workaround like it's implemented in this patch and improve
   commit message to clarify all potential doubts/questions from the user
   perspective.

What's your thoughts?

Kind regards

Grzegorz

> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > index 89bb8461284a..a5df081ffc19 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > @@ -2630,25 +2630,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw
> *hw, u8 port)
> >   	return 0;
> >   }
> >
> > -/**
> > - * ice_sb_access_ena_eth56g - Enable SB devices (PHY and others) access
> > - * @hw: pointer to HW struct
> > - * @enable: Enable or disable access
> > - *
> > - * Enable sideband devices (PHY and others) access.
> > - */
> > -static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
> > -{
> > -	u32 val = rd32(hw, PF_SB_REM_DEV_CTL);
> > -
> > -	if (enable)
> > -		val |= BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1);
> > -	else
> > -		val &= ~(BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1));
> > -
> > -	wr32(hw, PF_SB_REM_DEV_CTL, val);
> > -}
> > -
> >   /**
> >    * ice_ptp_init_phc_e825 - Perform E825 specific PHC initialization
> >    * @hw: pointer to HW struct
> > @@ -2659,8 +2640,6 @@ static void ice_sb_access_ena_eth56g(struct
> ice_hw *hw, bool enable)
> >    */
> >   static int ice_ptp_init_phc_e825(struct ice_hw *hw)
> >   {
> > -	ice_sb_access_ena_eth56g(hw, true);
> > -
> >   	/* Initialize the Clock Generation Unit */
> >   	return ice_init_cgu_e82x(hw);
> >   }
> > @@ -2747,8 +2726,6 @@ static void ice_ptp_init_phy_e825(struct ice_hw
> *hw)
> >   	params->num_phys = 2;
> >   	ptp->ports_per_phy = 4;
> >   	ptp->num_lports = params->num_phys * ptp->ports_per_phy;
> > -
> > -	ice_sb_access_ena_eth56g(hw, true);
> >   }
> >
> >   /* E822 family functions

RE: [PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

[Nitka, Grzegorz]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Tuesday, February 25, 2025 4:05 PM
> To: Nitka, Grzegorz <grzegorz.nitka@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kolacinski,
> Karol <karol.kolacinski@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Subject: Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on
> 2xNAC E825 devices
> 
> On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:
> > From: Karol Kolacinski <karol.kolacinski@intel.com>
> >
> > According to the E825C specification, SBQ address for ports on a single
> > complex is device 2 for PHY 0 and device 13 for PHY1.
> > For accessing ports on a dual complex E825C (so called 2xNAC mode),
> > the driver should use destination device 2 (referred as phy_0) for
> > the current complex PHY and device 13 (referred as phy_0_peer) for
> > peer complex PHY.
> >
> > Differentiate SBQ destination device by checking if current PF port
> > number is on the same PHY as target port number.
> >
> > Adjust 'ice_get_lane_number' function to provide unique port number for
> > ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
> > ports). Cache this value in ice_hw struct.
> >
> > Introduce ice_get_primary_hw wrapper to get access to timesync register
> > not available from second NAC.
> >
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> > Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice.h        | 60 ++++++++++++++++++++-
> >  drivers/net/ethernet/intel/ice/ice_common.c |  6 ++-
> >  drivers/net/ethernet/intel/ice/ice_ptp.c    | 49 ++++++++++++-----
> >  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++++++++++---
> >  drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  5 --
> >  drivers/net/ethernet/intel/ice/ice_type.h   |  1 +
> >  6 files changed, 134 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> > index 53b990e2e850..d80ab48402f1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -193,8 +193,6 @@
> >
> >  #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
> >
> > -#define ice_pf_src_tmr_owned(pf) ((pf)-
> >hw.func_caps.ts_func_info.src_tmr_owned)
> > -
> >  enum ice_feature {
> >  	ICE_F_DSCP,
> >  	ICE_F_PHY_RCLK,
> > @@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct
> ice_pf *pf)
> >  }
> >
> >  extern const struct xdp_metadata_ops ice_xdp_md_ops;
> > +
> > +/**
> > + * ice_is_dual - Check if given config is multi-NAC
> > + * @hw: pointer to HW structure
> > + *
> > + * Return: true if the device is running in mutli-NAC (Network
> > + * Acceleration Complex) configuration variant, false otherwise
> > + * (always false for non-E825 devices).
> > + */
> > +static inline bool ice_is_dual(struct ice_hw *hw)
> > +{
> > +	return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> > +	       (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
> > +}
> 
> Thanks for the complete Kernel doc, and not adding unnecessary () around
> the value returned. Overall these helpers seem nice and clean to me.
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> > index ed7ef8608cbb..4ff4c99d0872 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
> >  		}
> >  	}
> >
> > +	hw->lane_num = ice_get_phy_lane_number(hw);
> > +
> 
> Unfortunately there seems to be a bit of a problem here:
> 
> The type of hw->lane number is u8.
> But ice_get_phy_lane_number returns an int,
> which may be a negative error value...
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> ...

Hello Simon,

Thanks for your review and apologize for a late response (I was OOO last
week).
Yes, this is actually a good catch!
I have no idea how it happened, most likely my typo when backporting
the patch from our reference code. hw->lane_num is declared as 's8' there.
To be fixed in v2 (along with doc update from your other comment).

> 
> > @@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
> >  {
> >  	struct ice_ptp *ptp = &pf->ptp;
> >  	struct ice_hw *hw = &pf->hw;
> > -	int lane_num, err;
> > +	int err;
> >
> >  	ptp->state = ICE_PTP_INITIALIZING;
> >
> > -	lane_num = ice_get_phy_lane_number(hw);
> > -	if (lane_num < 0) {
> > -		err = lane_num;
> > +	if (hw->lane_num < 0) {
> 
> ... which is checked here.
> 
> But because hw->lane_num is unsigned, this condition is always false.
> 
> FWIIW, I think it is nice that that hw->lane is a u8.
> But error handling seems broken here.
> 
> > +		err = hw->lane_num;
> >  		goto err_exit;
> >  	}
> > +	ptp->port.port_num = hw->lane_num;
> >
> > -	ptp->port.port_num = (u8)lane_num;
> >  	ice_ptp_init_hw(hw);
> >
> >  	ice_ptp_init_tx_interrupt_mode(pf);
> 
> ...

RE: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

[Nitka, Grzegorz]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Nitka, Grzegorz
> Sent: Tuesday, March 4, 2025 2:04 PM
> To: Paul Menzel <pmenzel@molgen.mpg.de>; Kolacinski, Karol
> <karol.kolacinski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> band access workaround for E825
> 
> > -----Original Message-----
> > From: Paul Menzel <pmenzel@molgen.mpg.de>
> > Sent: Friday, February 21, 2025 10:16 PM
> > To: Nitka, Grzegorz <grzegorz.nitka@intel.com>; Kolacinski, Karol
> > <karol.kolacinski@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski
> > <michal.swiatkowski@linux.intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> > band access workaround for E825
> >
> > Dear Grzegorz, dear Karol,
> >
> >
> > Thank you for your patch.
> >
> > Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> > > From: Karol Kolacinski <karol.kolacinski@intel.com>
> > >
> > > Due to the bug in FW/NVM autoload mechanism (wrong default
> > > SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> > > clients was disabled by default.
> >
> > I’d add a blank line between the paragraphs.
> >
> > > As the workaround solution, the register value was overwritten by the
> > > driver at the probe or reset handling.
> > > Remove workaround as it's not needed anymore. The fix in autoload
> > > procedure has been provided with NVM 3.80 version.
> >
> > Is this compatible with Linux’ no regression policy? People might only
> > update the Linux kernel and not the firmware.
> >
> > How did you test this, and how can others test this?
> >
> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> > > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
> > >   1 file changed, 23 deletions(-)
> >
> >
> > Kind regards,
> >
> > Paul
> >
> >
> 
> Dear Paul, first of all thank you for your review and apologize for late
> response (simply, I was out previous week).
> 
> Removing that workaround was a conscious decision.
> Rationale for that is, that the 'problematic' workaround was provided
> in very early product development stage (~ year ago).  Now, especially
> when E825-C product was just officially announced, the customer shall
> use official, approved SW ingredients.
> 
> Here are more details on this:
> - introduction to E825-C devices was provided in kernel 6.6, to allow
>   selected customers for early E825-C enablement. At that time E825-C
>   product family was in early phase (kind of Alpha), and one of the
>   consequences, from today's perspective,  is that it included 'unwanted'
>   workarounds like this.
> 
> - this change applies to E825-C products only, which is SoC product, not
>   a regular NIC card.  What I'd like to emphasize here, it requires even more
>   customer support from Intel company in terms of providing matching
>   SW stack (like BIOS, firmware, drivers etc.).
> 
> I see two possibilities now:
> 1) if the regression policy you mentioned is inviolable, keep the workaround
>    in the kernel code like it is today. Maybe we could add NVM version
> checker
>    and apply register updates for older NVMs only.
>    But, in my opinion, it would lead to keeping a dead code. There shouldn't
> be
>    official FW (I hope I won't regret these words) on the market which
> requires
>    this workaround.
> 
> 2) remove the workaround like it's implemented in this patch and improve
>    commit message to clarify all potential doubts/questions from the user
>    perspective.
> 
> What's your thoughts?
> 
> Kind regards
> 
> Grzegorz
> 

I've just submitted v2 of this series, but no changes in this area yet (except adding
blank line suggestion)
I'm waiting for feedback or confirmation if above justification is sufficient.
I'll submit one more if needed.

Regards

Grzegorz

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

[Simon Horman]

On Mon, Mar 10, 2025 at 12:36:31PM +0000, Nitka, Grzegorz wrote:
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Nitka, Grzegorz
> > Sent: Tuesday, March 4, 2025 2:04 PM
> > To: Paul Menzel <pmenzel@molgen.mpg.de>; Kolacinski, Karol
> > <karol.kolacinski@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski
> > <michal.swiatkowski@linux.intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> > band access workaround for E825
> > 
> > > -----Original Message-----
> > > From: Paul Menzel <pmenzel@molgen.mpg.de>
> > > Sent: Friday, February 21, 2025 10:16 PM
> > > To: Nitka, Grzegorz <grzegorz.nitka@intel.com>; Kolacinski, Karol
> > > <karol.kolacinski@intel.com>
> > > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kitszel,
> > > Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski
> > > <michal.swiatkowski@linux.intel.com>
> > > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> > > band access workaround for E825
> > >
> > > Dear Grzegorz, dear Karol,
> > >
> > >
> > > Thank you for your patch.
> > >
> > > Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> > > > From: Karol Kolacinski <karol.kolacinski@intel.com>
> > > >
> > > > Due to the bug in FW/NVM autoload mechanism (wrong default
> > > > SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> > > > clients was disabled by default.
> > >
> > > I’d add a blank line between the paragraphs.
> > >
> > > > As the workaround solution, the register value was overwritten by the
> > > > driver at the probe or reset handling.
> > > > Remove workaround as it's not needed anymore. The fix in autoload
> > > > procedure has been provided with NVM 3.80 version.
> > >
> > > Is this compatible with Linux’ no regression policy? People might only
> > > update the Linux kernel and not the firmware.
> > >
> > > How did you test this, and how can others test this?
> > >
> > > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> > > > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > > > ---
> > > >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
> > > >   1 file changed, 23 deletions(-)
> > >
> > >
> > > Kind regards,
> > >
> > > Paul
> > >
> > >
> > 
> > Dear Paul, first of all thank you for your review and apologize for late
> > response (simply, I was out previous week).
> > 
> > Removing that workaround was a conscious decision.
> > Rationale for that is, that the 'problematic' workaround was provided
> > in very early product development stage (~ year ago).  Now, especially
> > when E825-C product was just officially announced, the customer shall
> > use official, approved SW ingredients.
> > 
> > Here are more details on this:
> > - introduction to E825-C devices was provided in kernel 6.6, to allow
> >   selected customers for early E825-C enablement. At that time E825-C
> >   product family was in early phase (kind of Alpha), and one of the
> >   consequences, from today's perspective,  is that it included 'unwanted'
> >   workarounds like this.
> > 
> > - this change applies to E825-C products only, which is SoC product, not
> >   a regular NIC card.  What I'd like to emphasize here, it requires even more
> >   customer support from Intel company in terms of providing matching
> >   SW stack (like BIOS, firmware, drivers etc.).
> > 
> > I see two possibilities now:
> > 1) if the regression policy you mentioned is inviolable, keep the workaround
> >    in the kernel code like it is today. Maybe we could add NVM version
> > checker
> >    and apply register updates for older NVMs only.
> >    But, in my opinion, it would lead to keeping a dead code. There shouldn't
> > be
> >    official FW (I hope I won't regret these words) on the market which
> > requires
> >    this workaround.
> > 
> > 2) remove the workaround like it's implemented in this patch and improve
> >    commit message to clarify all potential doubts/questions from the user
> >    perspective.
> > 
> > What's your thoughts?
> > 
> > Kind regards
> > 
> > Grzegorz
> > 
> 
> I've just submitted v2 of this series, but no changes in this area yet (except adding
> blank line suggestion)
> I'm waiting for feedback or confirmation if above justification is sufficient.
> I'll submit one more if needed.

Hi Grzegorz,

Sorry for not commenting on this earlier,
your question has been hanging out on the ML for a while now.

From my point of view the key part of your justification is that
Intel has sufficient control of the SW stack and availability of (SoC)
devices such that in practice users would not see a regression.
And in my view this is entirely reasonable and the approach taken by
this patch is fine.

I do, however, think some text regarding this belongs in the patch
description. And I'll respond in that vein to v3 [*].

[*] https://lore.kernel.org/netdev/20250310122439.3327908-2-grzegorz.nitka@intel.com/