Re: [Intel-wired-lan] [PATCH net-next 07/15] ice: check Tx timestamp memory register for ready timestamps

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

On 11/3/2022 1:45 PM, Stillwell Jr, Paul M wrote:
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob
>> Keller
>> Sent: Tuesday, November 1, 2022 3:53 PM
>> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
>> Subject: [Intel-wired-lan] [PATCH net-next 07/15] ice: check Tx timestamp
>> memory register for ready timestamps
>>
>> The PHY for e822 based hardware has a register which indicates which
>> timestamps are valid in the PHY timestamp memory block. Each bit in the
>> register indicates whether the associated index in the timestamp memory is
>> valid.
>>
>> Hardware sets this bit when the timestamp is captured, and clears the bit when
>> the timestamp is read. Use of this register is important as reading timestamp
>> registers can impact the way that hardware generates timestamp interrupts.
>>
>> This occurs because the PHY has an internal value which is incremented when
>> hardware captures a timestamp and decremented when software reads a
>> timestamp. Reading timestamps which are not marked as valid still decrement
>> the internal value and can result in the Tx timestamp interrupt not triggering in
>> the future.
>>
>> To prevent this, use the timestamp memory value to determine which
>> timestamps are ready to be read. The ice_get_phy_tx_tstamp_ready function
>> reads this value. For e810 devices, this just always returns with all bits set.
>>
>> Skip any timestamp which is not set in this bitmap, avoiding reading extra
>> timestamps on e822 devices.
>>
>> Modify the stale check to apply only to e810 devices. It is not necessary for e822
>> devices because the timestamp memory register will prevent us from reading a
>> stale timestamp.
>>
>> Modify the ICE_PTP_TS_VALID check to simply drop the timestamp immediately
>> so that in an event of getting such an invalid timestamp the driver does not
>> attempt to re-read the timestamp again in a future poll of the register.
>>
>> With these changes, the driver now reads each timestamp register exactly once,
>> and does not attempt any re-reads. This ensures the interrupt tracking logic in
>> the PHY will not get stuck.
>>
> 
> Minor nit: for the E8xx devices we should capitalize the E in the commit message
> (e.g. E822, E810, et al)


Yep. I'll try to fix all of these.

Thanks,
Jake

> 
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ptp.c    | 41 ++++++++++--
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 72 +++++++++++++++++++++
>> drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  1 +
>>   3 files changed, 108 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index ebe910326963..39c68a272c6a 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -655,6 +655,9 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>>   	struct ice_ptp_port *ptp_port;
>>   	bool ts_handled = true;
>>   	struct ice_pf *pf;
>> +	struct ice_hw *hw;
>> +	u64 tstamp_ready;
>> +	int err;
>>   	u8 idx;
>>
>>   	if (!tx->init)
>> @@ -662,6 +665,12 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>>
>>   	ptp_port = container_of(tx, struct ice_ptp_port, tx);
>>   	pf = ptp_port_to_pf(ptp_port);
>> +	hw = &pf->hw;
>> +
>> +	/* Read the Tx ready status first */
>> +	err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
>> +	if (err)
>> +		return false;
>>
>>   	for_each_set_bit(idx, tx->in_use, tx->len) {
>>   		struct skb_shared_hwtstamps shhwtstamps = {}; @@ -669,7
>> +678,6 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>>   		u64 raw_tstamp, tstamp;
>>   		bool drop_ts = false;
>>   		struct sk_buff *skb;
>> -		int err;
>>
>>   		/* Drop packets which have waited for more than 2 seconds */
>>   		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) { @@
>> -677,24 +685,45 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>>
>>   			/* Count the number of Tx timestamps that timed out
>> */
>>   			pf->ptp.tx_hwtstamp_timeouts++;
>> +		}
>>
>> -			goto skip_ts_read;
>> +		/* Only read a timestamp from the PHY if its marked as ready
>> +		 * by the tstamp_ready register. This avoids unnecessary
>> +		 * reading of timestamps which are not yet valid. This is
>> +		 * important as we must read all timestamps which are valid
>> +		 * and only timestamps which are valid during each interrupt.
>> +		 * If we do not, the hardware logic for generating a new
>> +		 * interrupt can get stuck on some devices.
>> +		 */
>> +		if (!(tstamp_ready & BIT_ULL(phy_idx))) {
>> +			if (drop_ts)
>> +				goto skip_ts_read;
>> +			else
>> +				continue;
>>   		}
>>
>>   		ice_trace(tx_tstamp_fw_req, tx->tstamps[idx].skb, idx);
>>
>> -		err = ice_read_phy_tstamp(&pf->hw, tx->block, phy_idx,
>> -					  &raw_tstamp);
>> +		err = ice_read_phy_tstamp(hw, tx->block, phy_idx,
>> &raw_tstamp);
>>   		if (err)
>>   			continue;
>>
>>   		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
>>
>> -		/* Check if the timestamp is invalid or stale */
>> -		if (!(raw_tstamp & ICE_PTP_TS_VALID) ||
>> +		/* For e810 hardware, the tstamp_ready bitmask does not
> 
> Same comment about capital E in comment
> 
>> +		 * indicate whether a timestamp is ready. Instead, we check to
>> +		 * make sure the timestamp is different from the previous
>> +		 * cached value. If it is not, we need to re-read the
>> +		 * timestamp later until we get a valid value.
>> +		 */
>> +		if (!drop_ts && ice_is_e810(hw) &&
>>   		    raw_tstamp == tx->tstamps[idx].cached_tstamp)
>>   			continue;
>>
>> +		/* Discard any timestamp value without the valid bit set */
>> +		if (!(raw_tstamp & ICE_PTP_TS_VALID))
>> +			drop_ts = true;
>> +
>>   skip_ts_read:
>>   		spin_lock(&tx->lock);
>>   		tx->tstamps[idx].cached_tstamp = raw_tstamp; diff --git
>> a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 6c149b88c235..55bbe76ce0bd 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> @@ -2417,6 +2417,43 @@ int ice_phy_calc_vernier_e822(struct ice_hw *hw,
>> u8 port)
>>   	return 0;
>>   }
>>
>> +/**
>> + * ice_get_phy_tx_tstamp_ready_e822 - Read Tx memory status register
>> + * @hw: pointer to the HW struct
>> + * @quad: the timestamp quad to read from
>> + * @tstamp_ready: contents of the Tx memory status register
>> + *
>> + * Read the Q_REG_TX_MEMORY_STATUS register indicating which timestamps
>> +in
>> + * the PHY are ready. A set bit means the corresponding timestamp is
>> +valid and
>> + * ready to be captured from the PHY timestamp block.
>> + */
>> +static int
>> +ice_get_phy_tx_tstamp_ready_e822(struct ice_hw *hw, u8 quad, u64
>> +*tstamp_ready) {
>> +	u32 hi, lo;
>> +	int err;
>> +
>> +	err = ice_read_quad_reg_e822(hw, quad,
>> Q_REG_TX_MEMORY_STATUS_U,
>> +					&hi);
>> +	if (err) {
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to read
>> TX_MEMORY_STATUS_U for quad %u, err %d\n",
>> +			  quad, err);
>> +		return err;
>> +	}
>> +
>> +	err = ice_read_quad_reg_e822(hw, quad,
>> Q_REG_TX_MEMORY_STATUS_L,
>> +					&lo);
>> +	if (err) {
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to read
>> TX_MEMORY_STATUS_L for quad %u, err %d\n",
>> +			  quad, err);
>> +		return err;
>> +	}
>> +
>> +	*tstamp_ready = (u64)hi << 32 | (u64)lo;
>> +
>> +	return 0;
>> +}
>> +
>>   /* E810 functions
>>    *
>>    * The following functions operate on the E810 series devices which use @@ -
>> 3091,6 +3128,21 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8
>> idx)
>>   		return ice_clear_phy_tstamp_e822(hw, block, idx);  }
>>
>> +/* ice_get_phy_tx_tstamp_ready_e810 - Read Tx memory status register
>> + * @hw: pointer to the HW struct
>> + * @port: the PHY port to read
>> + * @tstamp_ready: contents of the Tx memory status register
>> + *
>> + * E810 devices do not use a Tx memory status register. Instead simply
>> + * indicate that all timestamps are currently ready.
>> + */
>> +static int
>> +ice_get_phy_tx_tstamp_ready_e810(struct ice_hw *hw, u8 port, u64
>> +*tstamp_ready) {
>> +	*tstamp_ready = 0xFFFFFFFFFFFFFFFF;
>> +	return 0;
>> +}
>> +
>>   /* E810T SMA functions
>>    *
>>    * The following functions operate specifically on E810T hardware and are used
>> @@ -3306,3 +3358,23 @@ int ice_ptp_init_phc(struct ice_hw *hw)
>>   	else
>>   		return ice_ptp_init_phc_e822(hw);
>>   }
>> +
>> +/* ice_get_phy_tx_tstamp_ready - Read PHY Tx memory status indication
>> + * @hw: pointer to the HW struct
>> + * @block: the timestamp block to check
>> + * @tstamp_ready: storage for the PHY Tx memory status information
>> + *
>> + * Check the PHY for Tx timestamp memory status. This reports a 64 bit
>> +value
>> + * which indicates which timestamps in the block may be captured. A set
>> +bit
>> + * means the timestamp can be read. An unset bit means the timestamp is
>> +not
>> + * ready and software should avoid reading the register.
>> + */
>> +int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64
>> +*tstamp_ready) {
>> +	if (ice_is_e810(hw))
>> +		return ice_get_phy_tx_tstamp_ready_e810(hw, block,
>> +							tstamp_ready);
>> +	else
>> +		return ice_get_phy_tx_tstamp_ready_e822(hw, block,
>> +							tstamp_ready);
>> +}
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>> index b0cd73aaac6b..b781dadf5a39 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
>> @@ -135,6 +135,7 @@ int ice_read_phy_tstamp(struct ice_hw *hw, u8 block,
>> u8 idx, u64 *tstamp);  int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8
>> idx);  void ice_ptp_reset_ts_memory(struct ice_hw *hw);  int
>> ice_ptp_init_phc(struct ice_hw *hw);
>> +int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64
>> +*tstamp_ready);
>>
>>   /* E822 family functions */
>>   int ice_read_phy_reg_e822(struct ice_hw *hw, u8 port, u16 offset, u32 *val);
>> --
>> 2.38.0.83.gd420dda05763
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan