> -----Original Message----- > From: Jacob Keller <[email protected]> > Sent: Saturday, September 26, 2026 1:57 AM > To: Intel Wired LAN <[email protected]>; Machnikowski, > Maciej <[email protected]>; Keller, Jacob E > <[email protected]>; Korba, Przemyslaw > <[email protected]>; Nguyen, Anthony L > <[email protected]>; Nitka, Grzegorz > <[email protected]>; Kubalewski, Arkadiusz > <[email protected]> > Cc: Keller, Jacob E <[email protected]>; Machnikowski, Maciej > <[email protected]> > Subject: [PATCH iwl-net v3 09/15] ice: E825: clear > PHY_REG_TX_MEMORY_STATUS prior to soft reset > > The current implementation of ice_ptp_reset_ts_memory_eth56g() 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. > > If a read fails (i.e. due to a transient sideband queue failure) it is > not treated as a fatal error. There is already a dynamic debug message > for each read failure. Keep track of the total number of timestamp > registers that fail for a given port and log a warning message > indicating the total number of failures. Continuing is acceptable as > clearing the memory is a precaution against faulty software. Correct > software access won't read the index twice and won't read it at all > except when it has already confirmed that the index is valid by > reading ice_get_phy_tx_tstamp_ready(). > > Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at > initialization") > Reviewed-by: Maciek Machnikowski <[email protected]> > Signed-off-by: Jacob Keller <[email protected]> > --- > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 100 +++++++++++++++---- > --------- > 1 file changed, 52 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > index 07b55fbb88dd..e83ad2bb8d42 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 > @@ -1134,16 +1116,12 @@ static int ice_read_ptp_tstamp_eth56g(struct > ice_hw *hw, u8 port, u8 idx, > * @port: the quad to read from > * @idx: the timestamp index to reset > * > - * Read and then forcibly clear the timestamp index to ensure the > valid bit is > - * cleared and the timestamp status bit is reset in the PHY port > memory of > - * internal PHYs of the 56G devices. > + * Read the timestamp index to ensure that the valid bit is cleared > and > + the > + * timestamp status bit is reset in the PHY port memory. > * > - * 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(). > - * > - * This function should only be called on an idx whose bit is set > according to > - * ice_get_phy_tx_tstamp_ready(). > + * This function should only be called on an index whose bit is set > + according > + * to ice_get_phy_tx_tstamp_ready(), or as part of a full sweep when > + paired > + * with a PHY soft reset via ice_ptp_phy_soft_reset_eth56g(). > * > * Return: > * * %0 - success > @@ -1152,24 +1130,16 @@ 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) { > ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY > timestamp register for port %u, idx %u, err %d\n", > 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; > } > > @@ -1177,19 +1147,43 @@ static int ice_clear_ptp_tstamp_eth56g(struct > ice_hw *hw, u8 port, u8 idx) } > > /** > - * 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. > + * > + * Failure to read a given index (i.e. due to a transient sideband > + queue > + * failure) is not considered fatal as the PHY port is about to be > soft reset. > + * While the soft reset does not clear the timestamp memory, software > + * shouldn't be reading the timestamp memory without already knowing > it > + is > + * valid via ice_get_phy_tx_tstamp_ready(), and this sweep is just > + * a precaution to ensure the memory is in a known state. > */ > -static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw) > +static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, > u8 > +port) > { > - unsigned int port; > + int err, failed = 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) > + failed++; > } > + > + if (failed) > + dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY > timestamp registers for port %u\n", > + failed, port); > } > > /** > @@ -2295,6 +2289,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 > @@ -2303,6 +2298,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. > */ > @@ -2311,6 +2312,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw > *hw, u8 port) > u32 global_val; > int err; > > + ice_ptp_clear_tx_memory_status_eth56g(hw, port); > + > 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", @@ -5802,8 +5805,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.56.0.rc0.395.gd1f3524e15dc
Reviewed-by: Aleksandr Loktionov <[email protected]>
