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

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

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> The drivers 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.
> 
> 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 <maciej.machnikowski@intel.com>
> ---
>  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;
> -	}
> -

Sashiko says:

> Does ice_clear_ptp_tstamp_eth56g() intentionally ignore the error from
> ice_read_ptp_tstamp_eth56g() and unconditionally return 0?
> If the read fails, the timestamp memory valid bits won't be cleared, which
> could leave stale bits intact before the soft reset.
> Additionally, unconditionally returning 0 here makes the error check in the
> newly introduced ice_ptp_clear_tx_memory_status_eth56g() loop dead code:
> 	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> 		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
> 		if (err)
> 			return err; /* This is never reached */
> 	}
> Should ice_clear_ptp_tstamp_eth56g() return err instead of 0 when the PHY
> timestamp read fails?

It makes sense to me that we should bail here if we fail to access the
device. Presumably multiple other accesses would be failing but I think
its not correct to hide the error in the clear timestamp function. Will
fix in v2.