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:44 PM, Tony Nguyen wrote:
> On 11/1/2022 3:52 PM, Jacob Keller wrote:
>> 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.
>>
>> 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;
> 
> Technically this else isn't needed since the previous condition is a goto
> 

True its not strictly needed, and I guess its more clear to have it or 
not. I guess our practice is to drop them. Will fix.


>>           }
>>           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
>> +         * 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);
> 
> CHECK: Alignment should match open parenthesis
> 

Will fix.

> 
>> +    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);
> 
> CHECK: Alignment should match open parenthesis
> 

Will fix.

> 
>> +    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
> 
> /**
>   * ice_get_phy_tx_tstamp_ready_e810 - Read Tx memory status register
> 

Ah, yep, will fix.

>> + * @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
> 
> This one too

Yep, thanks.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan