Re: [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly

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

On 7/19/2023 10:56 PM, Paul Menzel wrote:
> Dear Jacob, dear Karol,
> 
> 
> Thank you for the patch. Some minor things, only if you care.
> 
> Am 20.07.23 um 00:05 schrieb Jacob Keller:
>> From: Karol Kolacinski <karol.kolacinski@intel.com>
>>
>> E822 PHY TS registers should not be written and the only way to cleanup
>> them is to reset QUAD memory.
> 
> The verb *clean up* is spelled with a space.
> 
> … to clean them up …
> 
> Also in the commit message summary/title: Clean up …
> 
>> To ensure that the status bit for the timestamp index is cleared, ensure
>> that ice_clear_phy_tstamp implementations first read the timestamp out.
>> Implementations which can write the register continue to do so.
>>
>> Add a note to indicate this function should only be called on timestamps
>> which have their valid bit set.
> 
> Maybe also mention the extended debug messages.
> 
>> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 66 ++++++++++++---------
>>   1 file changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index f174bac58aba..6cab87595690 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> @@ -633,34 +633,31 @@ ice_read_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx, u64 *tstamp)
>>   }
>>   
>>   /**
>> - * ice_clear_phy_tstamp_e822 - Clear a timestamp from the quad block
>> + * ice_clear_phy_tstamp_e822 - Drop a timestamp from the quad block
>>    * @hw: pointer to the HW struct
>>    * @quad: the quad to read from
>>    * @idx: the timestamp index to reset
>>    *
>> - * Clear a timestamp, resetting its valid bit, from the PHY quad block that is
>> - * shared between the internal PHYs on the E822 devices.
>> + * Read the timetamp out of the quad to clear its timestamp status bit from
> 
> timestamp

Will fix.

> 
>> + * the PHY quad block that is shared between the internal PHYs of the E822
>> + * devices.
>> + *
>> + * Note that software cannot directly write the quad memory bank registers,
>> + * and must use ice_ptp_reset_ts_memory_quad_e822 for that purpose.
> 
> I’d add () to functions.

Will fix.

> 
>> + *
>> + * This function should only be called on an idx whose bit is set according to
>> + * ice_get_phy_tx_tstamp_ready.
>>    */
>>   static int
>>   ice_clear_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx)
>>   {
>> -	u16 lo_addr, hi_addr;
>> +	u64 unused_tstamp;
>>   	int err;
>>   
>> -	lo_addr = (u16)TS_L(Q_REG_TX_MEMORY_BANK_START, idx);
>> -	hi_addr = (u16)TS_H(Q_REG_TX_MEMORY_BANK_START, idx);
>> -
>> -	err = ice_write_quad_reg_e822(hw, quad, lo_addr, 0);
>> +	err = ice_read_phy_tstamp_e822(hw, quad, idx, &unused_tstamp);
>>   	if (err) {
>> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n",
>> -			  err);
>> -		return err;
>> -	}
>> -
>> -	err = ice_write_quad_reg_e822(hw, quad, hi_addr, 0);
>> -	if (err) {
>> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n",
>> -			  err);
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for quad %u, idx %u, err %d\n",
>> +			  quad, idx, err);
>>   		return err;
>>   	}
>>   
>> @@ -2657,28 +2654,39 @@ ice_read_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
>>    * @lport: the lport to read from
>>    * @idx: the timestamp index to reset
>>    *
>> - * Clear a timestamp, resetting its valid bit, from the timestamp block of the
>> - * external PHY on the E810 device.
>> + * Read the timestamp and then forcibly overwrite its value to clear the valid
>> + * bit from the timestamp block of the external PHY on the E810 device.
>> + *
>> + * This function should only be called on an idx whose bit is set according to
>> + * ice_get_phy_tx_tstamp_ready.
>>    */
>>   static int ice_clear_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx)
>>   {
>>   	u32 lo_addr, hi_addr;
>> +	u64 unused_tstamp;
>>   	int err;
>>   
>> +	err = ice_read_phy_tstamp_e810(hw, lport, idx, &unused_tstamp);
>> +	if (err) {
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for lport %u, idx %u, err %d\n",
>> +			  lport, idx, err);
>> +		return err;
>> +	}
>> +
>>   	lo_addr = TS_EXT(LOW_TX_MEMORY_BANK_START, lport, idx);
>>   	hi_addr = TS_EXT(HIGH_TX_MEMORY_BANK_START, lport, idx);
>>   
>>   	err = ice_write_phy_reg_e810(hw, lo_addr, 0);
>>   	if (err) {
>> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n",
>> -			  err);
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for lport %u, idx %u, err %d\n",
>> +			  lport, idx, err);
>>   		return err;
>>   	}
>>   
>>   	err = ice_write_phy_reg_e810(hw, hi_addr, 0);
>>   	if (err) {
>> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n",
>> -			  err);
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register for lport %u, idx %u, err %d\n",
>> +			  lport, idx, err);
>>   		return err;
>>   	}
>>   
>> @@ -3326,14 +3334,18 @@ int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp)
>>   }
>>   
>>   /**
>> - * ice_clear_phy_tstamp - Clear a timestamp from the timestamp block
>> + * ice_clear_phy_tstamp - Drop a timestamp from the timestamp block
>>    * @hw: pointer to the HW struct
>>    * @block: the block to read from
>>    * @idx: the timestamp index to reset
>>    *
>> - * Clear a timestamp, resetting its valid bit, from the timestamp block. For
>> - * E822 devices, the block is the quad to clear from. For E810 devices, the
>> - * block is the logical port to clear from.
>> + * Drop a timestamp from the timestamp block by reading it. This will reset
>> + * the memory status bit allowing the timestamp index to be reused. For E822
>> + * devices, the block is the quad to clear from. For E810 devices, the block
>> + * is the logical port to clear from.
>> + *
>> + * This function should only be called on a timestamp index whose valid bit
>> + * is set according to ice_get_phy_tx_tstamp_ready.
>>    */
>>   int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx)
>>   {
> 
> As the function name still contains *clear*, I am unsure, what the 
> difference between *drop* and *clear* is.
> 

I'll try to clarify this in v2.

It turns out that one of my colleagues believes this last change should
be on net as a fix, so I am going to send that first.

Thanks,
Jake

> 
> Kind regards,
> 
> Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan