Re: [PATCH net-next v3 1/5] eth: fbnic: add software TX timestamping support

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

On 10/7/2024 2:56 AM, Vadim Fedorenko wrote:
> On 04/10/2024 23:55, Jacob Keller wrote:
>>
>>
>> On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:
>>> Add software TX timestamping support. RX software timestamping is
>>> implemented in the core and there is no need to provide special flag
>>> in the driver anymore.
>>>
>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>> ---
>>>   drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 11 +++++++++++
>>>   drivers/net/ethernet/meta/fbnic/fbnic_txrx.c    |  3 +++
>>>   2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>>> index 5d980e178941..ffc773014e0f 100644
>>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>>> @@ -6,6 +6,16 @@
>>>   #include "fbnic_netdev.h"
>>>   #include "fbnic_tlv.h"
>>>   
>>> +static int
>>> +fbnic_get_ts_info(struct net_device *netdev,
>>> +		  struct kernel_ethtool_ts_info *tsinfo)
>>> +{
>>> +	tsinfo->so_timestamping =
>>> +		SOF_TIMESTAMPING_TX_SOFTWARE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>
>> You could use ethtool_op_get_ts_info(), but I imagine future patches
>> will update this for hardware timestamping, so I don't think thats a big
>> deal.
>>
>> I think you *do* still want to report SOF_TIMESTAMPING_RX_SOFTWARE and
>> SOF_TIMESTAMPING_SOFTWARE to get the API correct... Perhaps that could
>> be improved in the core stack though.... Or did that already get changed
>> recently?
> 
> Yeah, as you found in the next mail, software RX timestamping was moved
> to the core recently.
> 
>> You should also set phc_index to -1 until you have a PTP clock device.
> 
> That's definitely missing, thanks! I'll add it to the next version.
> 

Since I had missed this, I reviewed the patch that changed Rx timestamp
to the core. It also initialized PHC index to -1 automatically so it
shouldn't be necessary.

Apologies for the misleading comment.