[0/3] some bugfixes for PTP

  The patchset include some bugfixes for PTP.

Huisong Li (3):
  examples/ptpclient: add the check for PTP capability
  net/hns3: fix fail to receive PTP packet
  ethdev: add the check for the valitity of timestamp offload

 drivers/net/hns3/hns3_ptp.c      |  1 -
 drivers/net/hns3/hns3_rxtx_vec.c | 22 +++++------
 examples/ptpclient/ptpclient.c   |  5 +++
 lib/ethdev/rte_ethdev.c          | 65 +++++++++++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 14 deletions(-)

--
2.22.0
  

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.

PTP is a protocol for time sync.
Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf.

> 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.

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.

We just need to clarify TIMESTAMP offload and PTP usage and find out
what is causing confusion.
I would be great if you can help on clarification, and update
documentation or API comments, or what ever required, for this.

> ---
> 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(-)
>
  

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.
>
> 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.
>
>> 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.
There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3 
and so on.
>
> 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(-)
>>
> .
  

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.
> 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(-)
>>>
>> .
  

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.  

We added patch in ptpclient earlier to pass the timestamp offload, however later we also updated the driver to do it by default. 


> > 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(-)
> >>>
> >> .
  

add ice & igc maintainers

在 2023/9/21 19:06, Ferruh Yigit 写道:
> 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"?
It indicates whether the device supports PTP or enable  PTP feature.
If TIMESTAMP offload is not for PTP, I don't know what the point of this 
offload independent existence is.
>
>>> 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.
understand.
>
>>>> 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.
>> 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?

*-->igc code:*

Having following codes in igc_recv_scattered_pkts():

         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
                     uint32_t *, -IGC_TS_HDR_LEN);
             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
                     ts[2];
             rxm->timesync = rxq->queue_id;
         }
Note：this rxm->timesync will be used in timesync_read_rx_timestamp()

*-->ice code:*

#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
         if (ice_timestamp_dynflag > 0 &&
             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             if (unlikely(is_tsinit)) {
                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1, 
rxq->time_high);
                 rxq->hw_time_low = (uint32_t)ts_ns;
                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
                 is_tsinit = false;
             } else {
                 if (rxq->time_high < rxq->hw_time_low)
                     rxq->hw_time_high += 1;
                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
                 rxq->hw_time_low = rxq->time_high;
             }
             rxq->hw_time_update = rte_get_timer_cycles() /
                          (rte_get_timer_hz() / 1000);
             *RTE_MBUF_DYNFIELD(rxm,
                        (ice_timestamp_dynfield_offset),
                        rte_mbuf_timestamp_t *) = ts_ns;
             pkt_flags |= ice_timestamp_dynflag;
         }

         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
             RTE_PTYPE_L2_ETHER_TIMESYNC)) {
             rxq->time_high =
                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
             rxm->timesync = rxq->queue_id;
             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
         }
#endif

Note: rxm->timesync and rxq->time_high will be used in 
timesync_read_rx_timestamp()

>
>
>>> 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(-)
>>>>
>>> .
> .
  

在 2023/9/21 19:17, Hemant Agrawal 写道:
> 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.
>
> We added patch in ptpclient earlier to pass the timestamp offload, however later we also updated the driver to do it by default.
>
It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
Actually, whether PTP code is compiled should not depended on this macro 
RTE_LIBRTE_IEEE1588.
If there is a capability, it will be perfect, no matter whether it is 
TIMESTAMP offload.
What do you think, Ferruh?
>>> 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(-)
>>>>>
>>>> .
  

On 10/20/2023 4:58 AM, lihuisong (C) wrote:
> 
> 在 2023/9/21 19:17, Hemant Agrawal 写道:
>> 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.
>>
>> We added patch in ptpclient earlier to pass the timestamp offload,
>> however later we also updated the driver to do it by default.
>>
>>
> It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
> Actually, whether PTP code is compiled should not depended on this macro
> RTE_LIBRTE_IEEE1588.
>

There is already a patch by Thomas to remove RTE_LIBRTE_IEEE1588 [1],
agree that this functionality needs some attention.

Removing RTE_LIBRTE_IEEE1588 impact drivers, that is what holding us back.


[1]
https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/

> If there is a capability, it will be perfect, no matter whether it is
> TIMESTAMP offload.
> What do you think, Ferruh?
>

Difficulty is to know when to enable HW timestamp, and for some drivers
this may change the descriptor format (to include timestamp), so driver
should set correct datapath functions for this case.

We know when a HW timer is required, it is required for PTP protocol and
required for TIMESTAMP offload.

What do you think to dynamically enable it for PTP when
'rte_eth_timesync_enable()' API called, and for TIMESTAMP offload when
the offload is enabled.
If this works, now new configuration item or offload is required, what
do you think?


>>>> 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(-)
>>>>>>
>>>>> .
  

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(-)
>>>>>
>>>> .
>
  

timesync_read_rx_timestamp
On 9/21/2023 12:59 PM, lihuisong (C) wrote:
> add ice & igc maintainers
> 
> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>> 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"?
>>
> It indicates whether the device supports PTP or enable  PTP feature.
>

We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
APIs to control PTP support.

But when mention from "offload", it is something device itself does.

PTP is a protocol (IEEE 1588), and used to synchronize clocks.
What I get is protocol can be parsed by networking stack and it can be
used by application to synchronize clock.

When you are refer to "PTP offload", does it mean device (NIC)
understands the protocol and parse it to synchronize device clock with
other devices?


We have 'rte_eth_timesync_*()' APIs, my understanding is application
parses the PTP protocol, and it may use this information to configure
NIC to synchronize its clock, but it may also use PTP provided
information to sync any other clock. Is this understanding correct?


> If TIMESTAMP offload is not for PTP, I don't know what the point of this
> offload independent existence is.
>

TIMESTAMP offload request device to add timestamp to mbuf in ingress,
and use mbuf timestamp to schedule packet for egress.

Technically this time-stamping can be done by driver, but if offload
set, HW timestamp is used for it.

Rx timestamp can be used for various reasons, like debugging and
performance/latency analyses, etc..


>>
>>>> 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.
> understand.
>>
>>>>> 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.
>>> 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?
> 
> *-->igc code:*
> 
> Having following codes in igc_recv_scattered_pkts():
> 
>         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>                     uint32_t *, -IGC_TS_HDR_LEN);
>             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>                     ts[2];
>             rxm->timesync = rxq->queue_id;
>         }
> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
> 

Above code requires TIMESTAMP offload to set timesync, but this
shouldn't be a requirement. Usage seems mixed.

> *-->ice code:*
> 
> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>         if (ice_timestamp_dynflag > 0 &&
>             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>             rxq->time_high =
>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>             if (unlikely(is_tsinit)) {
>                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
> rxq->time_high);
>                 rxq->hw_time_low = (uint32_t)ts_ns;
>                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>                 is_tsinit = false;
>             } else {
>                 if (rxq->time_high < rxq->hw_time_low)
>                     rxq->hw_time_high += 1;
>                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
>                 rxq->hw_time_low = rxq->time_high;
>             }
>             rxq->hw_time_update = rte_get_timer_cycles() /
>                          (rte_get_timer_hz() / 1000);
>             *RTE_MBUF_DYNFIELD(rxm,
>                        (ice_timestamp_dynfield_offset),
>                        rte_mbuf_timestamp_t *) = ts_ns;
>             pkt_flags |= ice_timestamp_dynflag;
>         }
> 
>         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>             RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>             rxq->time_high =
>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>             rxm->timesync = rxq->queue_id;
>             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>         }
> #endif
> 
> Note: rxm->timesync and rxq->time_high will be used in
> timesync_read_rx_timestamp()
> 

This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
and flag set accordingly.

And if PTP enabled, and PTP packet type detected relevant flag set in
mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'.


Although above usage looks correct, I can see in 'ice_timesync_enable()'
TIMESTAMP offload is used requirement to enable timesync.
TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
mentioned what is done is dpaa2.


>>
>>
>>>> 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(-)
>>>>>
>>>> .
>> .
  

在 2023/11/2 7:39, Ferruh Yigit 写道:
> On 10/20/2023 4:58 AM, lihuisong (C) wrote:
>> 在 2023/9/21 19:17, Hemant Agrawal 写道:
>>> 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.
>>>
>>> We added patch in ptpclient earlier to pass the timestamp offload,
>>> however later we also updated the driver to do it by default.
>>>
>>>
>> It is a little mess for PTP and RTE_LIBRTE_IEEE1588 to use.
>> Actually, whether PTP code is compiled should not depended on this macro
>> RTE_LIBRTE_IEEE1588.
>>
> There is already a patch by Thomas to remove RTE_LIBRTE_IEEE1588 [1],
> agree that this functionality needs some attention.
>
> Removing RTE_LIBRTE_IEEE1588 impact drivers, that is what holding us back.
+1 remove the compile macro RTE_LIBRTE_IEEE1588.
And hns3 had beed removed it.
>
>
> [1]
> https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/
>
>> If there is a capability, it will be perfect, no matter whether it is
>> TIMESTAMP offload.
>> What do you think, Ferruh?
>>
> Difficulty is to know when to enable HW timestamp, and for some drivers
> this may change the descriptor format (to include timestamp), so driver
> should set correct datapath functions for this case.
Yes, to get Rx timestamp of PTP packet from descriptor for many NIC.
>
> We know when a HW timer is required, it is required for PTP protocol and
> required for TIMESTAMP offload.
TIMESTAMP offload may be unnecessary for some NIC which don't get Rx 
timestamp from descriptor(But, IMO, like this hardware is very rare.).
>
> What do you think to dynamically enable it for PTP when
> 'rte_eth_timesync_enable()' API called, and for TIMESTAMP offload when
> the offload is enabled.
Agree above.
At least, this can make sure all NIC can enable PTP feature.
> If this works, now new configuration item or offload is required, what
> do you think?
The new capability item is required to know if the port support PTP feature.
so application can enable/disable PTP based on this capability.
>
>>>>> 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(-)
>>>>>>>
>>>>>> .
> .
  

在 2023/11/2 7:39, Ferruh Yigit 写道:
> timesync_read_rx_timestamp
> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>> add ice & igc maintainers
>>
>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>> 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"?
>>>
>> It indicates whether the device supports PTP or enable  PTP feature.
>>
> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
> APIs to control PTP support.
No, this is just to control it.
we still need to like a device capablity to report application if the 
port support to call this API, right?
>
> But when mention from "offload", it is something device itself does.
>
> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
> What I get is protocol can be parsed by networking stack and it can be
> used by application to synchronize clock.
>
> When you are refer to "PTP offload", does it mean device (NIC)
> understands the protocol and parse it to synchronize device clock with
> other devices?
Good point. PTP offload is unreasonable.
But the capablity is required indeed.
What do you think of introducing a RTE_ETH_DEV_PTP in 
dev->data->dev_flags for PTP feature?
>
>
> We have 'rte_eth_timesync_*()' APIs, my understanding is application
> parses the PTP protocol, and it may use this information to configure
> NIC to synchronize its clock, but it may also use PTP provided
> information to sync any other clock. Is this understanding correct?
>
>
>> If TIMESTAMP offload is not for PTP, I don't know what the point of this
>> offload independent existence is.
>>
> TIMESTAMP offload request device to add timestamp to mbuf in ingress,
> and use mbuf timestamp to schedule packet for egress.
Agree.
>
> Technically this time-stamping can be done by driver, but if offload
> set, HW timestamp is used for it.
>
> Rx timestamp can be used for various reasons, like debugging and
> performance/latency analyses, etc..
>
>
>>>>> 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.
>> understand.
>>>>>> 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.
>>>> 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?
>> *-->igc code:*
>>
>> Having following codes in igc_recv_scattered_pkts():
>>
>>          if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>>              uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>>                      uint32_t *, -IGC_TS_HDR_LEN);
>>              rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>>                      ts[2];
>>              rxm->timesync = rxq->queue_id;
>>          }
>> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
>>
> Above code requires TIMESTAMP offload to set timesync, but this
> shouldn't be a requirement. Usage seems mixed.
>
>> *-->ice code:*
>>
>> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>>          if (ice_timestamp_dynflag > 0 &&
>>              (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>>              rxq->time_high =
>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>              if (unlikely(is_tsinit)) {
>>                  ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
>> rxq->time_high);
>>                  rxq->hw_time_low = (uint32_t)ts_ns;
>>                  rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>>                  is_tsinit = false;
>>              } else {
>>                  if (rxq->time_high < rxq->hw_time_low)
>>                      rxq->hw_time_high += 1;
>>                  ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high;
>>                  rxq->hw_time_low = rxq->time_high;
>>              }
>>              rxq->hw_time_update = rte_get_timer_cycles() /
>>                           (rte_get_timer_hz() / 1000);
>>              *RTE_MBUF_DYNFIELD(rxm,
>>                         (ice_timestamp_dynfield_offset),
>>                         rte_mbuf_timestamp_t *) = ts_ns;
>>              pkt_flags |= ice_timestamp_dynflag;
>>          }
>>
>>          if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>>              RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>>              rxq->time_high =
>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>              rxm->timesync = rxq->queue_id;
>>              pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>          }
>> #endif
>>
>> Note: rxm->timesync and rxq->time_high will be used in
>> timesync_read_rx_timestamp()
>>
> This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
> and flag set accordingly.
hns3 also implemented PTP as ice did.
>
> And if PTP enabled, and PTP packet type detected relevant flag set in
> mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'.
Yes.
>
>
> Although above usage looks correct, I can see in 'ice_timesync_enable()'
> TIMESTAMP offload is used requirement to enable timesync.
> TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
> mentioned what is done is dpaa2.
>
>
>>>
>>>>> 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(-)
>>>>>>
>>>>> .
>>> .
> .
  

Hi Ferruh,

在 2023/11/23 19:56, lihuisong (C) 写道:
>
> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>> timesync_read_rx_timestamp
>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>> add ice & igc maintainers
>>>
>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>> 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"?
>>>>
>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>
>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>> APIs to control PTP support.
> No, this is just to control it.
> we still need to like a device capablity to report application if the 
> port support to call this API, right?
>>
>> But when mention from "offload", it is something device itself does.
>>
>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>> What I get is protocol can be parsed by networking stack and it can be
>> used by application to synchronize clock.
>>
>> When you are refer to "PTP offload", does it mean device (NIC)
>> understands the protocol and parse it to synchronize device clock with
>> other devices?
> Good point. PTP offload is unreasonable.
> But the capablity is required indeed.
> What do you think of introducing a RTE_ETH_DEV_PTP in 
> dev->data->dev_flags for PTP feature?

Can you take a look at this discussion line again?

Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if 
the device support PTP feature.

>>
>>
>> We have 'rte_eth_timesync_*()' APIs, my understanding is application
>> parses the PTP protocol, and it may use this information to configure
>> NIC to synchronize its clock, but it may also use PTP provided
>> information to sync any other clock. Is this understanding correct?
>>
>>
>>> If TIMESTAMP offload is not for PTP, I don't know what the point of 
>>> this
>>> offload independent existence is.
>>>
>> TIMESTAMP offload request device to add timestamp to mbuf in ingress,
>> and use mbuf timestamp to schedule packet for egress.
> Agree.
>>
>> Technically this time-stamping can be done by driver, but if offload
>> set, HW timestamp is used for it.
>>
>> Rx timestamp can be used for various reasons, like debugging and
>> performance/latency analyses, etc..
>>
>>
>>>>>> 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.
>>> understand.
>>>>>>> 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.
>>>>> 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?
>>> *-->igc code:*
>>>
>>> Having following codes in igc_recv_scattered_pkts():
>>>
>>>          if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>>>              uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg,
>>>                      uint32_t *, -IGC_TS_HDR_LEN);
>>>              rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC +
>>>                      ts[2];
>>>              rxm->timesync = rxq->queue_id;
>>>          }
>>> Note：this rxm->timesync will be used in timesync_read_rx_timestamp()
>>>
>> Above code requires TIMESTAMP offload to set timesync, but this
>> shouldn't be a requirement. Usage seems mixed.
>>
>>> *-->ice code:*
>>>
>>> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
>>>          if (ice_timestamp_dynflag > 0 &&
>>>              (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) {
>>>              rxq->time_high =
>>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>>              if (unlikely(is_tsinit)) {
>>>                  ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1,
>>> rxq->time_high);
>>>                  rxq->hw_time_low = (uint32_t)ts_ns;
>>>                  rxq->hw_time_high = (uint32_t)(ts_ns >> 32);
>>>                  is_tsinit = false;
>>>              } else {
>>>                  if (rxq->time_high < rxq->hw_time_low)
>>>                      rxq->hw_time_high += 1;
>>>                  ts_ns = (uint64_t)rxq->hw_time_high << 32 | 
>>> rxq->time_high;
>>>                  rxq->hw_time_low = rxq->time_high;
>>>              }
>>>              rxq->hw_time_update = rte_get_timer_cycles() /
>>>                           (rte_get_timer_hz() / 1000);
>>>              *RTE_MBUF_DYNFIELD(rxm,
>>>                         (ice_timestamp_dynfield_offset),
>>>                         rte_mbuf_timestamp_t *) = ts_ns;
>>>              pkt_flags |= ice_timestamp_dynflag;
>>>          }
>>>
>>>          if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) ==
>>>              RTE_PTYPE_L2_ETHER_TIMESYNC)) {
>>>              rxq->time_high =
>>>                 rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high);
>>>              rxm->timesync = rxq->queue_id;
>>>              pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>          }
>>> #endif
>>>
>>> Note: rxm->timesync and rxq->time_high will be used in
>>> timesync_read_rx_timestamp()
>>>
>> This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field
>> and flag set accordingly.
> hns3 also implemented PTP as ice did.
>>
>> And if PTP enabled, and PTP packet type detected relevant flag set in
>> mbuf, and timesyc value set to use later for 
>> 'timesync_read_rx_timestamp()'.
> Yes.
>>
>>
>> Although above usage looks correct, I can see in 'ice_timesync_enable()'
>> TIMESTAMP offload is used requirement to enable timesync.
>> TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant
>> mentioned what is done is dpaa2.
>>
>>
>>>>
>>>>>> 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(-)
>>>>>>>
>>>>>> .
>>>> .
>> .
>
> .
  

On 1/11/2024 6:25 AM, lihuisong (C) wrote:
> Hi Ferruh,
> 
> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>
>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>> timesync_read_rx_timestamp
>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>> add ice & igc maintainers
>>>>
>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>> 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"?
>>>>>
>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>
>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>> APIs to control PTP support.
>> No, this is just to control it.
>> we still need to like a device capablity to report application if the
>> port support to call this API, right?
>>>
>>> But when mention from "offload", it is something device itself does.
>>>
>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>> What I get is protocol can be parsed by networking stack and it can be
>>> used by application to synchronize clock.
>>>
>>> When you are refer to "PTP offload", does it mean device (NIC)
>>> understands the protocol and parse it to synchronize device clock with
>>> other devices?
>> Good point. PTP offload is unreasonable.
>> But the capablity is required indeed.
>> What do you think of introducing a RTE_ETH_DEV_PTP in
>> dev->data->dev_flags for PTP feature?
> 
> Can you take a look at this discussion line again?
> 
> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
> the device support PTP feature.
> 

Hi Huisong,

First let me try to summarize the discussion since it has been some time.

HW timer block can be used for Rx timestamp offload
(RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
different things.

This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
support, which is wrong.



After we agreed on above, your next question is to use 'dev_flag' to
report PTP capability.

First, can you please describe what is the motivation to learn if HW
supports PTP or now, what is the benefit of knowing this.

If we agree that there is a need to know the PTP capability, question is
where to report this capability.

Suggested 'dev_flags' is used for various things, some are internal
flags and some are status, I don't think overloading this variable is
not good idea.

Other option is an update 'rte_eth_dev_info_get()' for it but it has the
same problem, this function is already overloaded with many different
things.

We can have an API just to get PTP capability, but this will require a
new dev_ops, this can be an option but my concern is having a capability
dev_ops for each feature create a mess in dev_ops.

Perhaps we can have single get_capability() API, and it gets features as
flags to return if that feature is supported or not, but this requires a
wider discussion.

Instead can we deduce the capability from PTP relevant dev_ops, if they
are implemented we can say it is supported? This doesn't require new
support.
  

在 2024/1/27 0:54, Ferruh Yigit 写道:
> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>> timesync_read_rx_timestamp
>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>> add ice & igc maintainers
>>>>>
>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>> 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"?
>>>>>>
>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>
>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>> APIs to control PTP support.
>>> No, this is just to control it.
>>> we still need to like a device capablity to report application if the
>>> port support to call this API, right?
>>>> But when mention from "offload", it is something device itself does.
>>>>
>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>> What I get is protocol can be parsed by networking stack and it can be
>>>> used by application to synchronize clock.
>>>>
>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>> understands the protocol and parse it to synchronize device clock with
>>>> other devices?
>>> Good point. PTP offload is unreasonable.
>>> But the capablity is required indeed.
>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>> dev->data->dev_flags for PTP feature?
>> Can you take a look at this discussion line again?
>>
>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>> the device support PTP feature.
>>
Hi Ferruh,

Thanks for taking your time to reply.

> Hi Huisong,
>
> First let me try to summarize the discussion since it has been some time.
>
> HW timer block can be used for Rx timestamp offload
> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
> different things.
>
> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
> support, which is wrong.
>
correct.
>
> After we agreed on above, your next question is to use 'dev_flag' to
> report PTP capability.
>
> First, can you please describe what is the motivation to learn if HW
> supports PTP or now, what is the benefit of knowing this.
There are a couple of device which have the same driver on a platform or OS.
But just allow one device to support or be responsible for PTP feature.
The firmware will report a capability to tell the device if it is 
support PTP.
But, currently, driver doesn't know how to report user which device 
support PTP feature.

In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
Whether the device supports PTP is irrelevant to this macro.
>
> If we agree that there is a need to know the PTP capability, question is
> where to report this capability.
>
> Suggested 'dev_flags' is used for various things, some are internal
> flags and some are status, I don't think overloading this variable is
> not good idea.
Yes, we need to consider  carefully.
>
> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
> same problem, this function is already overloaded with many different
> things.
>
> We can have an API just to get PTP capability, but this will require a
> new dev_ops, this can be an option but my concern is having a capability
> dev_ops for each feature create a mess in dev_ops.
>
> Perhaps we can have single get_capability() API, and it gets features as
> flags to return if that feature is supported or not, but this requires a
> wider discussion.
>
> Instead can we deduce the capability from PTP relevant dev_ops, if they
> are implemented we can say it is supported? This doesn't require new
> support.
Thank you mentioning so many ways.
For the end of advice, I don't think it is appropriate.
Because we have to modify dynamically the pointer address of all PTP 
APIs in dev_ops on the above case.

How about we use rte_eth_dev_info.dev_capa to report PTP offload?
This is specifically used to report "Non-offload capabilities" according 
to its document.


>
> .
  

On 1/27/2024 1:52 AM, lihuisong (C) wrote:
> 
> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>> timesync_read_rx_timestamp
>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>> add ice & igc maintainers
>>>>>>
>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>> 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"?
>>>>>>>
>>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>>
>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>> APIs to control PTP support.
>>>> No, this is just to control it.
>>>> we still need to like a device capablity to report application if the
>>>> port support to call this API, right?
>>>>> But when mention from "offload", it is something device itself does.
>>>>>
>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>> What I get is protocol can be parsed by networking stack and it can be
>>>>> used by application to synchronize clock.
>>>>>
>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>> understands the protocol and parse it to synchronize device clock with
>>>>> other devices?
>>>> Good point. PTP offload is unreasonable.
>>>> But the capablity is required indeed.
>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>> dev->data->dev_flags for PTP feature?
>>> Can you take a look at this discussion line again?
>>>
>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>>> the device support PTP feature.
>>>
> Hi Ferruh,
> 
> Thanks for taking your time to reply.
> 
>> Hi Huisong,
>>
>> First let me try to summarize the discussion since it has been some time.
>>
>> HW timer block can be used for Rx timestamp offload
>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>> different things.
>>
>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>> support, which is wrong.
>>
> correct.
>>
>> After we agreed on above, your next question is to use 'dev_flag' to
>> report PTP capability.
>>
>> First, can you please describe what is the motivation to learn if HW
>> supports PTP or now, what is the benefit of knowing this.
> There are a couple of device which have the same driver on a platform or
> OS.
> But just allow one device to support or be responsible for PTP feature.
> The firmware will report a capability to tell the device if it is
> support PTP.
> But, currently, driver doesn't know how to report user which device
> support PTP feature.
> 

Driver can hold a private data that records if PTP supported by the
device or not, and according this value PTP dev_ops can return error or
success.

This may not be ideal but it lets user to know about the support status,
can this work?


> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
> Whether the device supports PTP is irrelevant to this macro.
>

Yes, I guess because both features use same HW, there is confusion there.

>>
>> If we agree that there is a need to know the PTP capability, question is
>> where to report this capability.
>>
>> Suggested 'dev_flags' is used for various things, some are internal
>> flags and some are status, I don't think overloading this variable is
>> not good idea.
> Yes, we need to consider  carefully.
>>
>> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
>> same problem, this function is already overloaded with many different
>> things.
>>
>> We can have an API just to get PTP capability, but this will require a
>> new dev_ops, this can be an option but my concern is having a capability
>> dev_ops for each feature create a mess in dev_ops.
>>
>> Perhaps we can have single get_capability() API, and it gets features as
>> flags to return if that feature is supported or not, but this requires a
>> wider discussion.
>>
>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>> are implemented we can say it is supported? This doesn't require new
>> support.
> Thank you mentioning so many ways.
> For the end of advice, I don't think it is appropriate.
> Because we have to modify dynamically the pointer address of all PTP
> APIs in dev_ops on the above case.
> 

I was thinking for the case application distinguish if PTP related
dev_ops set or not, but after your explanation I can see this won't help
for your case.
Because in your case PTP dev_ops implemented but some devices support it
and some don't, and you are looking for a way to distinguish it.

> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
> This is specifically used to report "Non-offload capabilities" according
> to its document.
> 

As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
cautious to expand it more.
Also I think it is a wider discussion if we want a capability reporting
in ethdev and where it should be.
  

在 2024/1/29 19:16, Ferruh Yigit 写道:
> On 1/27/2024 1:52 AM, lihuisong (C) wrote:
>> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>>> timesync_read_rx_timestamp
>>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>>> add ice & igc maintainers
>>>>>>>
>>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>>> 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"?
>>>>>>>>
>>>>>>> It indicates whether the device supports PTP or enable  PTP feature.
>>>>>>>
>>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>>> APIs to control PTP support.
>>>>> No, this is just to control it.
>>>>> we still need to like a device capablity to report application if the
>>>>> port support to call this API, right?
>>>>>> But when mention from "offload", it is something device itself does.
>>>>>>
>>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>>> What I get is protocol can be parsed by networking stack and it can be
>>>>>> used by application to synchronize clock.
>>>>>>
>>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>>> understands the protocol and parse it to synchronize device clock with
>>>>>> other devices?
>>>>> Good point. PTP offload is unreasonable.
>>>>> But the capablity is required indeed.
>>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>>> dev->data->dev_flags for PTP feature?
>>>> Can you take a look at this discussion line again?
>>>>
>>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if
>>>> the device support PTP feature.
>>>>
>> Hi Ferruh,
>>
>> Thanks for taking your time to reply.
>>
>>> Hi Huisong,
>>>
>>> First let me try to summarize the discussion since it has been some time.
>>>
>>> HW timer block can be used for Rx timestamp offload
>>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>>> different things.
>>>
>>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>>> support, which is wrong.
>>>
>> correct.
>>> After we agreed on above, your next question is to use 'dev_flag' to
>>> report PTP capability.
>>>
>>> First, can you please describe what is the motivation to learn if HW
>>> supports PTP or now, what is the benefit of knowing this.
>> There are a couple of device which have the same driver on a platform or
>> OS.
>> But just allow one device to support or be responsible for PTP feature.
>> The firmware will report a capability to tell the device if it is
>> support PTP.
>> But, currently, driver doesn't know how to report user which device
>> support PTP feature.
>>
> Driver can hold a private data that records if PTP supported by the
> device or not, and according this value PTP dev_ops can return error or
> success.
>
> This may not be ideal but it lets user to know about the support status,
> can this work?
I don't think it is user friendly.
Users know which port supports the PTP feature only when the API fails 
to be invoked, right?
In addition, this is a common issue for all supported PTP device. So It 
is better to do this check in PMD.
>
>
>> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code flow.
>> Whether the device supports PTP is irrelevant to this macro.
>>
> Yes, I guess because both features use same HW, there is confusion there.
>
>>> If we agree that there is a need to know the PTP capability, question is
>>> where to report this capability.
>>>
>>> Suggested 'dev_flags' is used for various things, some are internal
>>> flags and some are status, I don't think overloading this variable is
>>> not good idea.
>> Yes, we need to consider  carefully.
>>> Other option is an update 'rte_eth_dev_info_get()' for it but it has the
>>> same problem, this function is already overloaded with many different
>>> things.
>>>
>>> We can have an API just to get PTP capability, but this will require a
>>> new dev_ops, this can be an option but my concern is having a capability
>>> dev_ops for each feature create a mess in dev_ops.
>>>
>>> Perhaps we can have single get_capability() API, and it gets features as
>>> flags to return if that feature is supported or not, but this requires a
>>> wider discussion.
>>>
>>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>>> are implemented we can say it is supported? This doesn't require new
>>> support.
>> Thank you mentioning so many ways.
>> For the end of advice, I don't think it is appropriate.
>> Because we have to modify dynamically the pointer address of all PTP
>> APIs in dev_ops on the above case.
>>
> I was thinking for the case application distinguish if PTP related
> dev_ops set or not, but after your explanation I can see this won't help
> for your case.
> Because in your case PTP dev_ops implemented but some devices support it
> and some don't, and you are looking for a way to distinguish it.
Yes
>
>> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
>> This is specifically used to report "Non-offload capabilities" according
>> to its document.
>>
> As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
> cautious to expand it more.
> Also I think it is a wider discussion if we want a capability reporting
> in ethdev and where it should be.
How about we send a RFC patch which use rte_eth_dev_info.dev_capa to 
report PTP offload and start to disscuss this issue?
And add Thomas's patch [1] and this patch.

[1]https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/

>
> .
  

On 1/29/2024 1:58 PM, lihuisong (C) wrote:
> 
> 在 2024/1/29 19:16, Ferruh Yigit 写道:
>> On 1/27/2024 1:52 AM, lihuisong (C) wrote:
>>> 在 2024/1/27 0:54, Ferruh Yigit 写道:
>>>> On 1/11/2024 6:25 AM, lihuisong (C) wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> 在 2023/11/23 19:56, lihuisong (C) 写道:
>>>>>> 在 2023/11/2 7:39, Ferruh Yigit 写道:
>>>>>>> timesync_read_rx_timestamp
>>>>>>> On 9/21/2023 12:59 PM, lihuisong (C) wrote:
>>>>>>>> add ice & igc maintainers
>>>>>>>>
>>>>>>>> 在 2023/9/21 19:06, Ferruh Yigit 写道:
>>>>>>>>> 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"?
>>>>>>>>>
>>>>>>>> It indicates whether the device supports PTP or enable  PTP
>>>>>>>> feature.
>>>>>>>>
>>>>>>> We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()'
>>>>>>> APIs to control PTP support.
>>>>>> No, this is just to control it.
>>>>>> we still need to like a device capablity to report application if the
>>>>>> port support to call this API, right?
>>>>>>> But when mention from "offload", it is something device itself does.
>>>>>>>
>>>>>>> PTP is a protocol (IEEE 1588), and used to synchronize clocks.
>>>>>>> What I get is protocol can be parsed by networking stack and it
>>>>>>> can be
>>>>>>> used by application to synchronize clock.
>>>>>>>
>>>>>>> When you are refer to "PTP offload", does it mean device (NIC)
>>>>>>> understands the protocol and parse it to synchronize device clock
>>>>>>> with
>>>>>>> other devices?
>>>>>> Good point. PTP offload is unreasonable.
>>>>>> But the capablity is required indeed.
>>>>>> What do you think of introducing a RTE_ETH_DEV_PTP in
>>>>>> dev->data->dev_flags for PTP feature?
>>>>> Can you take a look at this discussion line again?
>>>>>
>>>>> Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to
>>>>> reveal if
>>>>> the device support PTP feature.
>>>>>
>>> Hi Ferruh,
>>>
>>> Thanks for taking your time to reply.
>>>
>>>> Hi Huisong,
>>>>
>>>> First let me try to summarize the discussion since it has been some
>>>> time.
>>>>
>>>> HW timer block can be used for Rx timestamp offload
>>>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP) and/or PTP support, but they are two
>>>> different things.
>>>>
>>>> This patch uses 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' capability for PTP
>>>> support, which is wrong.
>>>>
>>> correct.
>>>> After we agreed on above, your next question is to use 'dev_flag' to
>>>> report PTP capability.
>>>>
>>>> First, can you please describe what is the motivation to learn if HW
>>>> supports PTP or now, what is the benefit of knowing this.
>>> There are a couple of device which have the same driver on a platform or
>>> OS.
>>> But just allow one device to support or be responsible for PTP feature.
>>> The firmware will report a capability to tell the device if it is
>>> support PTP.
>>> But, currently, driver doesn't know how to report user which device
>>> support PTP feature.
>>>
>> Driver can hold a private data that records if PTP supported by the
>> device or not, and according this value PTP dev_ops can return error or
>> success.
>>
>> This may not be ideal but it lets user to know about the support status,
>> can this work?
> I don't think it is user friendly.
> Users know which port supports the PTP feature only when the API fails
> to be invoked, right?
> In addition, this is a common issue for all supported PTP device. So It
> is better to do this check in PMD.
>>
>>
>>> In addition, many drivers use RTE_LIBRTE_IEEE1588 to control PTP code
>>> flow.
>>> Whether the device supports PTP is irrelevant to this macro.
>>>
>> Yes, I guess because both features use same HW, there is confusion there.
>>
>>>> If we agree that there is a need to know the PTP capability,
>>>> question is
>>>> where to report this capability.
>>>>
>>>> Suggested 'dev_flags' is used for various things, some are internal
>>>> flags and some are status, I don't think overloading this variable is
>>>> not good idea.
>>> Yes, we need to consider  carefully.
>>>> Other option is an update 'rte_eth_dev_info_get()' for it but it has
>>>> the
>>>> same problem, this function is already overloaded with many different
>>>> things.
>>>>
>>>> We can have an API just to get PTP capability, but this will require a
>>>> new dev_ops, this can be an option but my concern is having a
>>>> capability
>>>> dev_ops for each feature create a mess in dev_ops.
>>>>
>>>> Perhaps we can have single get_capability() API, and it gets
>>>> features as
>>>> flags to return if that feature is supported or not, but this
>>>> requires a
>>>> wider discussion.
>>>>
>>>> Instead can we deduce the capability from PTP relevant dev_ops, if they
>>>> are implemented we can say it is supported? This doesn't require new
>>>> support.
>>> Thank you mentioning so many ways.
>>> For the end of advice, I don't think it is appropriate.
>>> Because we have to modify dynamically the pointer address of all PTP
>>> APIs in dev_ops on the above case.
>>>
>> I was thinking for the case application distinguish if PTP related
>> dev_ops set or not, but after your explanation I can see this won't help
>> for your case.
>> Because in your case PTP dev_ops implemented but some devices support it
>> and some don't, and you are looking for a way to distinguish it.
> Yes
>>
>>> How about we use rte_eth_dev_info.dev_capa to report PTP offload?
>>> This is specifically used to report "Non-offload capabilities" according
>>> to its document.
>>>
>> As mentioned above 'rte_eth_dev_info' is overloaded, I am for being more
>> cautious to expand it more.
>> Also I think it is a wider discussion if we want a capability reporting
>> in ethdev and where it should be.
> How about we send a RFC patch which use rte_eth_dev_info.dev_capa to
> report PTP offload and start to disscuss this issue?
>

Ack. Lets start discussion on top of a patch. Thanks.

> And add Thomas's patch [1] and this patch.
> 
> [1]https://patchwork.dpdk.org/project/dpdk/patch/20230203132810.14187-1-thomas@monjalon.net/
> 
>>
>> .