On 9/21/2023 12:17 PM, Hemant Agrawal wrote:
> HI Ferruh,
>
>> On 9/21/2023 11:02 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> Sorry for my delay reply because of taking a look at all PMDs
>>> implementation.
>>>
>>>
>>> 在 2023/9/16 1:46, Ferruh Yigit 写道:
>>>> On 8/17/2023 9:42 AM, Huisong Li wrote:
>>>>> From the first version of ptpclient, it seems that this example
>>>>> assume that the PMDs support the PTP feature and enable PTP by
>>>>> default. Please see commit ab129e9065a5 ("examples/ptpclient: add
>>>>> minimal PTP client") which are introduced in 2015.
>>>>>
>>>>> And two years later, Rx HW timestamp offload was introduced to
>>>>> enable or disable PTP feature in HW via rte_eth_rxmode. Please see
>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability").
>>>>>
>>>> Hi Huisong,
>>>>
>>>> As far as I know this offload is not for PTP.
>>>> PTP and TIMESTAMP are different.
>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new
>>> offlaod for PTP.
>>>
>>
>> Can you please detail what is "PTP offload"?
>>
>>>>
>>>> PTP is a protocol for time sync.
>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.
>>> Yes.
>>> But a lot of PMDs actually depand on HW to report Rx timestamp
>>> releated information because of reading Rx timestamp of PTP SYNC
>>> packet in read_rx_timestamp API.
>>>
>>
>> HW support may be required for PTP but this doesn't mean timestamp
>> offload is used.
>
>>
>>>>
>>>>> And then about four years later, ptpclient enable Rx timestamp
>>>>> offload because some PMDs require this offload to enable. Please see
>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp
>> offload").
>>>>>
>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they
>>>> updated ptpclient sample to set TIMESTAMP offload.
>
> [Hemant] In case of dpaa2, we need to enable HW timestamp for PTP. In the
> current dpaa2 driver
> If the code is compiled with, RTE_LIBRTE_IEEE1588, we are enabling the HW
> timestamp
> Otherwise, we are only enabling it when the TIMESTAMP offload is selected.
>
I think this is reasonable, HW timestamp enabled only when required.
> We added patch in ptpclient earlier to pass the timestamp offload, however
> later we also updated the driver to do it by default.
>
This part I am not sure,
so application request TIMESTAMP offload enable HW timestamp to use it
for PTP.
There are already 'rte_eth_timesync_enable()' and
'rte_eth_timesync_disable()' functions, and ptpclient sample already
uses them, why now utilize these APIs to enable HW timestamp, or other
related configuration?
>
>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2,
>>> hns3 and so on.
>>>
>>
>> Can you please point the ice & igc code, cc'ing their maintainers, we can
>> look
>> together?
>>
>>
>>>>
>>>> We need to clarify dpaa2 usage.
>>>>
>>>>> By all the records, this is more like a process of perfecting PTP
>>>>> feature.
>>>>> Not all network adaptors support PTP feature. So adding the check
>>>>> for PTP capability in ethdev layer is necessary.
>>>>>
>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops
>>>> already checked, so no additional check is needed.
>>> But only having dev_ops about PTP doesn't satisfy the use of this feature.
>>> For example,
>>> there are serveal network ports belonged to a driver on one OS, and
>>> only one port support PTP function.
>>> So driver needs one *PTP* offload.
>>>>
>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out
>>>> what is causing confusion.
>>> Yes it is a little bit confusion.
>>> There are two kinds of implementation:
>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need
>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature.
>>> B: saving "Rx timestamp related information" from Rx description when
>>> receive PTP SYNC packet and
>>> report it in read_rx_timestamp API.
>>> For case B, most of driver use TIMESTAMP offload to decide if driver
>>> save "Rx timestamp related information.
>>> What do you think about this, Ferruh?
>>>> I would be great if you can help on clarification, and update
>>>> documentation or API comments, or what ever required, for this.
>>> ok
>>>>
>>>>> ---
>>>>> v3:
>>>>> - patch [2/3] for hns3 has been applied and so remove it.
>>>>> - ops pointer check is closer to usage.
>>>>>
>>>>> Huisong Li (2):
>>>>> examples/ptpclient: add the check for PTP capability
>>>>> ethdev: add the check for the valitity of timestamp offload
>>>>>
>>>>> examples/ptpclient/ptpclient.c | 5 +++
>>>>> lib/ethdev/rte_ethdev.c | 57
>>>>> +++++++++++++++++++++++++++++++++-
>>>>> 2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>> .
>
