Re: [PATCH iwl-net v2 09/15] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

[Jacob Keller <jacob.e.keller@intel.com>]

On 9/23/2026 2:41 AM, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Jacob Keller <jacob.e.keller@intel.com>
>> Sent: Tuesday, September 22, 2026 8:03 PM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>; Nitka, Grzegorz
>> <grzegorz.nitka@intel.com>; Kubalewski, Arkadiusz
>> <arkadiusz.kubalewski@intel.com>; Intel Wired LAN <intel-wired-
>> lan@lists.osuosl.org>; Machnikowski, Maciej
>> <maciej.machnikowski@intel.com>; Korba, Przemyslaw
>> <przemyslaw.korba@intel.com>; netdev@vger.kernel.org; Nguyen,
>> Anthony L <anthony.l.nguyen@intel.com>
>> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej
>> <maciej.machnikowski@intel.com>
>> 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 <maciej.machnikowski@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  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 <aleksandr.loktionov@intel.com>