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 <[email protected]>
> Reviewed-by: Maciek Machnikowski <[email protected]>
> ---
> 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.
