On 9/23/2026 2:41 AM, Loktionov, Aleksandr wrote: > > >> -----Original Message----- >> From: Jacob Keller <[email protected]> >> Sent: Tuesday, September 22, 2026 8:03 PM >> To: Keller, Jacob E <[email protected]>; Nitka, Grzegorz >> <[email protected]>; Kubalewski, Arkadiusz >> <[email protected]>; Intel Wired LAN <intel-wired- >> [email protected]>; Machnikowski, Maciej >> <[email protected]>; Korba, Przemyslaw >> <[email protected]>; [email protected]; Nguyen, >> Anthony L <[email protected]> >> Cc: Keller, Jacob E <[email protected]>; Machnikowski, Maciej >> <[email protected]> >> Subject: [PATCH iwl-net v2 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 c8a67a307832..e1a5ff5793d1 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); } >> > > ... > >> 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", >> + port, failed); > Failed, port arguments looks like swaped. >
Yep. Will fix. > > Reviewed-by: Aleksandr Loktionov <[email protected]>
