RE: [PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

is flawed. It tries to clear the timestamp memory by writing to the
PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it
does not trigger appropriate PHY actions.

To clear outstanding timestamp memory, the driver must read the
timestamps.
However, naively doing this as part of ice_ptp_reset_ts_memory() is
problematic. When reading the timestamp index, hardware kicks off a
chain of actions including clearing the ready bitmap index, and
decrementing an internal counter if the timestamp index was marked as
valid.

This can potentially leave the internal hardware counter out of sync
with the actual number of timestamps. This occurs because the
PHY_REG_TX_MEMORY_STATUS region is not zero-initialized when the
device boots up. Instead, it is filled with garbage. On a cold power
on, attempts to read the stale data result in the hardware triggering
a counter decrement for a timestamp that never happened. This
underflows the counter, and prevents new timestamp interrupts from
being triggered for real timestamp requests.

We must read the PHY_REG_TX_MEMORY_STATUS in order to clear stale
timestamps. But doing so may cause a desync with the counter. To
prevent issues, perform this clearing always and only right before
initiating a PHY soft reset.

The soft reset will clear and reset the internal counter and the ready
bitmap. The reads to PHY_REG_TX_MEMORY_STATUS will reset the region
valid bits ensuring that no stale data is left behind. This
combination ensures that we always have a clean slate with no stale
data and with the counter properly reset to zero.

Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at
initialization")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <redacted>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++-----
---------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index d48eb3c61823..b7d217ac31f3 100644

--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

@@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw

*hw, u8 port, u16 offset,
 	return ice_read_port_eth56g(hw, port, offset, val,
ETH56G_PHY_MEM_PTP);  }

-/**
- * ice_write_port_mem_eth56g - Write a PHY port memory location
- * @hw: pointer to the HW struct
- * @port: Port number to be read
- * @offset: Offset from PHY port register base
- * @val: Pointer to the value to read (out param)
- *
- * Return:
- * * %0      - success
- * * %EINVAL - invalid port number or resource type
- * * %other  - failed to write to PHY
- */
-static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16
offset,
-				     u32 val)
-{
-	return ice_write_port_eth56g(hw, port, offset, val,
ETH56G_PHY_MEM_PTP);
-}
-
 /**
  * ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
  * @hw: pointer to the HW struct

@@ -1139,8 +1121,8 @@ static int ice_read_ptp_tstamp_eth56g(struct

ice_hw *hw, u8 port, u8 idx,
  * internal PHYs of the 56G devices.
  *
  * To directly clear the contents of the timestamp block entirely,
discarding
- * all timestamp data at once, software should instead use
- * ice_ptp_reset_ts_memory_quad_eth56g().
+ * all timestamp data at once, software should instead perform a PHY
+ soft
+ * reset via ice_ptp_phy_soft_reset_eth56g().
  *
  * This function should only be called on an idx whose bit is set
according to
  * ice_get_phy_tx_tstamp_ready().

@@ -1152,11 +1134,11 @@ static int ice_read_ptp_tstamp_eth56g(struct

ice_hw *hw, u8 port, u8 idx,  static int
ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)  {
 	u64 unused_tstamp;
-	u16 lo_addr;
 	int err;

-	/* Read the timestamp register to ensure the timestamp status
bit is
-	 * cleared.
+	/* Per the PHY spec, reading the timestamp memory location is
what
+	 * clears the entry's valid bit and its corresponding (read-
only)
+	 * ts_memory_status bit.
 	 */
 	err = ice_read_ptp_tstamp_eth56g(hw, port, idx,
&unused_tstamp);
 	if (err) {

@@ -1164,32 +1146,40 @@ static int ice_clear_ptp_tstamp_eth56g(struct

ice_hw *hw, u8 port, u8 idx)
 			  port, idx, err);
 	}

-	lo_addr = (u16)PHY_TSTAMP_L(idx);
-
-	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
-	if (err) {
-		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP
timestamp register for port %u, idx %u, err %d\n",
-			  port, idx, err);
-		return err;
-	}
-
 	return 0;
 }

 /**
- * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the
port block
+ * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx
+ timestamp memory
  * @hw: pointer to the HW struct
+ * @port: port number to clear
+ *
+ * Fully reset a single PHY port's Tx timestamp memory. Per the PHY
+ spec, the
+ * only way to clear a timestamp valid bit (and its read-only
+ ts_memory_status
+ * bit) is to read the timestamp memory location, so read every entry
+ for the
+ * port (two 32-bit reads each). This discards all timestamp data on
+ the port,
+ * so it must only be used for a full reset; callers that must
preserve
+ * in-flight timestamps clear individual indices via
ice_clear_phy_tstamp().
+ *
+ * Due to interactions with an internal HW counter for the number of
+ * outstanding Tx timestamps, this *must* only be called as part of
the
+ * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal
+ counter
+ * may become out of sync and prevent new timestamp interrupts.
+ *
+ * Return: 0 on success, negative error code on failure to read the
PHY.
  */
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
+static int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw,
u8
+port)
 {
-	unsigned int port;
+	int err = 0;
+	u8 idx;

-	for (port = 0; port < hw->ptp.num_lports; port++) {
-		ice_write_ptp_reg_eth56g(hw, port,
PHY_REG_TX_MEMORY_STATUS_L,
-					 0);
-		ice_write_ptp_reg_eth56g(hw, port,
PHY_REG_TX_MEMORY_STATUS_U,
-					 0);
+	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
+		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
+		if (err)
+			return err;
 	}
+
+	return 0;
 }

 /**

@@ -2290,6 +2280,7 @@ int

ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32
*ts_status)
  *
  * Trigger a soft reset of the ETH56G PHY by toggling the soft reset
  * bit in the PHY global register. The reset sequence consists of:
+ *   0. Reading every timestamp memory register to clear its valid
bit
  *   1. Clearing the soft reset bit
  *   2. Asserting the soft reset bit
  *   3. Clearing the soft reset bit again

@@ -2298,6 +2289,12 @@ int

ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32
*ts_status)
  * to settle. This provides a controlled way to reinitialize the PHY
  * without requiring a full device reset.
  *
+ * To ensure that the internal counter matches the contents of the
+ * PHY_REG_TX_MEMORY_STATUS, read every timestamp index prior to
+ performing
+ * the soft reset. The PHY_REG_TX_MEMORY_STATUS reads ensure that the
+ region
+ * is cleared, while the soft reset procedure ensures that the
+ timestamp

+ * counter is reset to zero.
+ *
  * Return: 0 on success, or a negative error code on failure when
  *         reading or writing the PHY register.
  */

@@ -2306,6 +2303,13 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw

*hw, u8 port)
 	u32 global_val;
 	int err;

+	err = ice_ptp_clear_tx_memory_status_eth56g(hw, port);
+	if (err) {
+		ice_debug(hw, ICE_DBG_PTP, "Failed to clear
PHY_REG_TX_MEMORY_STATUS for port %d, err %d\n",
+			  port, err);
+		return err;
+	}
+
 	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL,
&global_val);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to read
PHY_REG_GLOBAL for port %d, err %d\n", @@ -5797,8 +5801,9 @@ void
ice_ptp_reset_ts_memory(struct ice_hw *hw)
 		ice_ptp_reset_ts_memory_e82x(hw);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		ice_ptp_reset_ts_memory_eth56g(hw);
-		break;
+		/* E825 hardware must only reset timestamp memory as
part of
+		 * the soft reset procedure.
+		 */
 	case ICE_MAC_E810:
 	default:
 		return;

--
2.55.0.814.gc42f45431d0f