[PATCH next-queue v5 0/4] igc: Add support for PCIe PTM

[PATCH next-queue v5 0/4] igc: Add support for PCIe PTM

[Vinicius Costa Gomes]

Hi,

Changes from v4:
  - Improved commit messages (Bjorn Helgaas);

Changes from v3:
  - More descriptive commit messages and comments (Bjorn Helgaas);
  - Added a pcie_ptm_enabled() helper (Bjorn Helgaas);

Changes from v2:
  - Now the PTM timestamps are retrieved synchronously with the
    ioctl();
  - Fixed some typos in constants;
  - The IGC_PTM_STAT register is write-1-to-clear, document this more
    clearly;

Changes from v1:
  - This now should cross compile better, convert_art_ns_to_tsc() will
    only be used if CONFIG_X86_TSC is enabled;
  - PCIe PTM errors reported by the NIC are logged and PTM cycles are
    restarted in case an error is detected;

Original cover letter (lightly edited):

This adds support for PCIe PTM (Precision Time Measurement) to the igc
driver. PCIe PTM allows the NIC and Host clocks to be compared more
precisely, improving the clock synchronization accuracy.

Patch 1/4 reverts a commit that made pci_enable_ptm() private to the
PCI subsystem, reverting makes it possible for it to be called from
the drivers.

Patch 2/4 adds the pcie_ptm_enabled() helper.

Patch 3/4 calls pci_enable_ptm() from the igc driver.

Patch 4/4 implements the PCIe PTM support. It adds a workqueue that
reads the PTM registers periodically and collects the information so a
subsequent call to getcrosststamp() has all the timestamps needed.

Some questions are raised (also pointed out in the commit message):

1. Using convert_art_ns_to_tsc() is too x86 specific, there should be
   a common way to create a 'system_counterval_t' from a timestamp.

2. convert_art_ns_to_tsc() says that it should only be used when
   X86_FEATURE_TSC_KNOWN_FREQ is true, but during tests it works even
   when it returns false. Should that check be done?

Cheers,

Vinicius Costa Gomes (4):
  Revert "PCI: Make pci_enable_ptm() private"
  PCI: Add pcie_ptm_enabled()
  igc: Enable PCIe PTM
  igc: Add support for PTP getcrosststamp()

 drivers/net/ethernet/intel/igc/igc.h         |   1 +
 drivers/net/ethernet/intel/igc/igc_defines.h |  31 ++++
 drivers/net/ethernet/intel/igc/igc_main.c    |   6 +
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 182 +++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  23 +++
 drivers/pci/pci.h                            |   3 -
 drivers/pci/pcie/ptm.c                       |   9 +
 include/linux/pci.h                          |  10 +
 8 files changed, 262 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH next-queue v5 1/4] Revert "PCI: Make pci_enable_ptm() private"

[Vinicius Costa Gomes]

Make pci_enable_ptm() accessible from the drivers.

Exposing this to the driver enables the driver to use the
'ptm_enabled' field of 'pci_dev' to check if PTM is enabled or not.

This reverts commit ac6c26da29c1 ("PCI: Make pci_enable_ptm() private").

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.h   | 3 ---
 include/linux/pci.h | 7 +++++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 37c913bbc6e1..32dab36c717e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -593,11 +593,8 @@ static inline void pcie_ecrc_get_policy(char *str) { }
 
 #ifdef CONFIG_PCIE_PTM
 void pci_ptm_init(struct pci_dev *dev);
-int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
 #else
 static inline void pci_ptm_init(struct pci_dev *dev) { }
-static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
-{ return -EINVAL; }
 #endif
 
 struct pci_dev_reset_methods {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59a57..a687dda262dd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1617,6 +1617,13 @@ static inline bool pci_aer_available(void) { return false; }
 
 bool pci_ats_disabled(void);
 
+#ifdef CONFIG_PCIE_PTM
+int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
+#else
+static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+{ return -EINVAL; }
+#endif
+
 void pci_cfg_access_lock(struct pci_dev *dev);
 bool pci_cfg_access_trylock(struct pci_dev *dev);
 void pci_cfg_access_unlock(struct pci_dev *dev);
-- 
2.31.1

[PATCH next-queue v5 2/4] PCI: Add pcie_ptm_enabled()

[Vinicius Costa Gomes]

Add a predicate that returns if PCIe PTM (Precision Time Measurement)
is enabled.

It will only return true if it's enabled in all the ports in the path
from the device to the root.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/ptm.c | 9 +++++++++
 include/linux/pci.h    | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 95d4eef2c9e8..8a4ad974c5ac 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -204,3 +204,12 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 	return 0;
 }
 EXPORT_SYMBOL(pci_enable_ptm);
+
+bool pcie_ptm_enabled(struct pci_dev *dev)
+{
+	if (!dev)
+		return false;
+
+	return dev->ptm_enabled;
+}
+EXPORT_SYMBOL(pcie_ptm_enabled);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a687dda262dd..fe7f264b2b15 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1619,9 +1619,12 @@ bool pci_ats_disabled(void);
 
 #ifdef CONFIG_PCIE_PTM
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
+bool pcie_ptm_enabled(struct pci_dev *dev);
 #else
 static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 { return -EINVAL; }
+static inline bool pcie_ptm_enabled(struct pci_dev *dev)
+{ return false; }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);
-- 
2.31.1

[PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Vinicius Costa Gomes]

Enables PCIe PTM (Precision Time Measurement) support in the igc
driver. Notifies the PCI devices that PCIe PTM should be enabled.

PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
in the PCIe fabric, it allows devices to report time measurements from
their internal clocks and the correlation with the PCIe root clock.

The i225 NIC exposes some registers that expose those time
measurements, those registers will be used, in later patches, to
implement the PTP_SYS_OFFSET_PRECISE ioctl().

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index a05e6d8ec660..f23d0303e53b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -12,6 +12,8 @@
 #include <net/pkt_sched.h>
 #include <linux/bpf_trace.h>
 #include <net/xdp_sock_drv.h>
+#include <linux/pci.h>
+
 #include <net/ipv6.h>
 
 #include "igc.h"
@@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
 
 	pci_enable_pcie_error_reporting(pdev);
 
+	err = pci_enable_ptm(pdev, NULL);
+	if (err < 0)
+		dev_err(&pdev->dev, "PTM not supported\n");
+
 	pci_set_master(pdev);
 
 	err = -ENOMEM;
-- 
2.31.1

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Paul Menzel]

Dear Vinicius,


Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
> Enables PCIe PTM (Precision Time Measurement) support in the igc
> driver. Notifies the PCI devices that PCIe PTM should be enabled.
> 
> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
> in the PCIe fabric, it allows devices to report time measurements from
> their internal clocks and the correlation with the PCIe root clock.
> 
> The i225 NIC exposes some registers that expose those time
> measurements, those registers will be used, in later patches, to
> implement the PTP_SYS_OFFSET_PRECISE ioctl().
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index a05e6d8ec660..f23d0303e53b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -12,6 +12,8 @@
>   #include <net/pkt_sched.h>
>   #include <linux/bpf_trace.h>
>   #include <net/xdp_sock_drv.h>
> +#include <linux/pci.h>
> +
>   #include <net/ipv6.h>
>   
>   #include "igc.h"
> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
>   
>   	pci_enable_pcie_error_reporting(pdev);
>   
> +	err = pci_enable_ptm(pdev, NULL);
> +	if (err < 0)
> +		dev_err(&pdev->dev, "PTM not supported\n");
> +

Sorry, if I am missing something, but do all devices supported by this 
driver support PTM or only the i225 NIC? In that case, it wouldn’t be an 
error for a device not supporting PTM, would it?

>   	pci_set_master(pdev);
>   
>   	err = -ENOMEM;
> 


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Vinicius Costa Gomes]

Hi Paul,

Paul Menzel <pmenzel@molgen.mpg.de> writes:

> Dear Vinicius,
>
>
> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
>> Enables PCIe PTM (Precision Time Measurement) support in the igc
>> driver. Notifies the PCI devices that PCIe PTM should be enabled.
>> 
>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
>> in the PCIe fabric, it allows devices to report time measurements from
>> their internal clocks and the correlation with the PCIe root clock.
>> 
>> The i225 NIC exposes some registers that expose those time
>> measurements, those registers will be used, in later patches, to
>> implement the PTP_SYS_OFFSET_PRECISE ioctl().
>> 
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index a05e6d8ec660..f23d0303e53b 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -12,6 +12,8 @@
>>   #include <net/pkt_sched.h>
>>   #include <linux/bpf_trace.h>
>>   #include <net/xdp_sock_drv.h>
>> +#include <linux/pci.h>
>> +
>>   #include <net/ipv6.h>
>>   
>>   #include "igc.h"
>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
>>   
>>   	pci_enable_pcie_error_reporting(pdev);
>>   
>> +	err = pci_enable_ptm(pdev, NULL);
>> +	if (err < 0)
>> +		dev_err(&pdev->dev, "PTM not supported\n");
>> +
>
> Sorry, if I am missing something, but do all devices supported by this 
> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an 
> error for a device not supporting PTM, would it?

That was a very good question. I had to talk with the hardware folks.
All the devices supported by the igc driver should support PTM.

And just to be clear, the reason that I am not returning an error here
is that PTM could not be supported by the host system (think PCI
controller).

>
>>   	pci_set_master(pdev);
>>   
>>   	err = -ENOMEM;
>> 
>
>
> Kind regards,
>
> Paul


Cheers,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Paul Menzel]

Dear Vinicius,


Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes:

> Paul Menzel writes:

>> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
>>> Enables PCIe PTM (Precision Time Measurement) support in the igc
>>> driver. Notifies the PCI devices that PCIe PTM should be enabled.
>>>
>>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
>>> in the PCIe fabric, it allows devices to report time measurements from
>>> their internal clocks and the correlation with the PCIe root clock.
>>>
>>> The i225 NIC exposes some registers that expose those time
>>> measurements, those registers will be used, in later patches, to
>>> implement the PTP_SYS_OFFSET_PRECISE ioctl().
>>>
>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>>> index a05e6d8ec660..f23d0303e53b 100644
>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>> @@ -12,6 +12,8 @@
>>>    #include <net/pkt_sched.h>
>>>    #include <linux/bpf_trace.h>
>>>    #include <net/xdp_sock_drv.h>
>>> +#include <linux/pci.h>
>>> +
>>>    #include <net/ipv6.h>
>>>    
>>>    #include "igc.h"
>>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
>>>    
>>>    	pci_enable_pcie_error_reporting(pdev);
>>>    
>>> +	err = pci_enable_ptm(pdev, NULL);
>>> +	if (err < 0)
>>> +		dev_err(&pdev->dev, "PTM not supported\n");
>>> +
>>
>> Sorry, if I am missing something, but do all devices supported by this
>> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an
>> error for a device not supporting PTM, would it?
> 
> That was a very good question. I had to talk with the hardware folks.
> All the devices supported by the igc driver should support PTM.

Thank you for checking that, that is valuable information.

> And just to be clear, the reason that I am not returning an error here
> is that PTM could not be supported by the host system (think PCI
> controller).

I just checked `pci_enable_ptm()` and on success it calls 
`pci_ptm_info()` logging a message:

	pci_info(dev, "PTM enabled%s, %s granularity\n",
		 dev->ptm_root ? " (root)" : "", clock_desc);

Was that present on your system with your patch? Please add that to the 
commit message.

Regarding my comment, I did not mean returning an error but the log 
*level* of the message. So, `dmesg --level err` would show that message. 
But if there are PCI controllers not supporting that, it’s not an error, 
but a warning at most. So, I’d use:

	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller 
(pci_enable_ptm() failed)\n");

>>>    	pci_set_master(pdev);
>>>    
>>>    	err = -ENOMEM;


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Vinicius Costa Gomes]

Paul Menzel <pmenzel@molgen.mpg.de> writes:

> Dear Vinicius,
>
>
> Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes:
>
>> Paul Menzel writes:
>
>>> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
>>>> Enables PCIe PTM (Precision Time Measurement) support in the igc
>>>> driver. Notifies the PCI devices that PCIe PTM should be enabled.
>>>>
>>>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
>>>> in the PCIe fabric, it allows devices to report time measurements from
>>>> their internal clocks and the correlation with the PCIe root clock.
>>>>
>>>> The i225 NIC exposes some registers that expose those time
>>>> measurements, those registers will be used, in later patches, to
>>>> implement the PTP_SYS_OFFSET_PRECISE ioctl().
>>>>
>>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>>>> index a05e6d8ec660..f23d0303e53b 100644
>>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>>> @@ -12,6 +12,8 @@
>>>>    #include <net/pkt_sched.h>
>>>>    #include <linux/bpf_trace.h>
>>>>    #include <net/xdp_sock_drv.h>
>>>> +#include <linux/pci.h>
>>>> +
>>>>    #include <net/ipv6.h>
>>>>    
>>>>    #include "igc.h"
>>>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
>>>>    
>>>>    	pci_enable_pcie_error_reporting(pdev);
>>>>    
>>>> +	err = pci_enable_ptm(pdev, NULL);
>>>> +	if (err < 0)
>>>> +		dev_err(&pdev->dev, "PTM not supported\n");
>>>> +
>>>
>>> Sorry, if I am missing something, but do all devices supported by this
>>> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an
>>> error for a device not supporting PTM, would it?
>> 
>> That was a very good question. I had to talk with the hardware folks.
>> All the devices supported by the igc driver should support PTM.
>
> Thank you for checking that, that is valuable information.
>
>> And just to be clear, the reason that I am not returning an error here
>> is that PTM could not be supported by the host system (think PCI
>> controller).
>
> I just checked `pci_enable_ptm()` and on success it calls 
> `pci_ptm_info()` logging a message:
>
> 	pci_info(dev, "PTM enabled%s, %s granularity\n",
> 		 dev->ptm_root ? " (root)" : "", clock_desc);
>
> Was that present on your system with your patch? Please add that to the 
> commit message.

Yes, with my patches applied I can see this message on my systems.

Sure, will add this to the commit message.

>
> Regarding my comment, I did not mean returning an error but the log 
> *level* of the message. So, `dmesg --level err` would show that message. 
> But if there are PCI controllers not supporting that, it’s not an error, 
> but a warning at most. So, I’d use:
>
> 	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller 
> (pci_enable_ptm() failed)\n");

I will use you suggestion for the message, but I think that warn is a
bit too much, info or notice seem to be better.


Cheers,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Paul Menzel]

Dear Vinicius,


Am 09.06.21 um 22:08 schrieb Vinicius Costa Gomes:
> Paul Menzel writes:

>> Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes:
>>
>>> Paul Menzel writes:
>>
>>>> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
>>>>> Enables PCIe PTM (Precision Time Measurement) support in the igc
>>>>> driver. Notifies the PCI devices that PCIe PTM should be enabled.
>>>>>
>>>>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running
>>>>> in the PCIe fabric, it allows devices to report time measurements from
>>>>> their internal clocks and the correlation with the PCIe root clock.
>>>>>
>>>>> The i225 NIC exposes some registers that expose those time
>>>>> measurements, those registers will be used, in later patches, to
>>>>> implement the PTP_SYS_OFFSET_PRECISE ioctl().
>>>>>
>>>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>>>> ---
>>>>>     drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++
>>>>>     1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>>>>> index a05e6d8ec660..f23d0303e53b 100644
>>>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>>>> @@ -12,6 +12,8 @@
>>>>>     #include <net/pkt_sched.h>
>>>>>     #include <linux/bpf_trace.h>
>>>>>     #include <net/xdp_sock_drv.h>
>>>>> +#include <linux/pci.h>
>>>>> +
>>>>>     #include <net/ipv6.h>
>>>>>     
>>>>>     #include "igc.h"
>>>>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev,
>>>>>     
>>>>>     	pci_enable_pcie_error_reporting(pdev);
>>>>>     
>>>>> +	err = pci_enable_ptm(pdev, NULL);
>>>>> +	if (err < 0)
>>>>> +		dev_err(&pdev->dev, "PTM not supported\n");
>>>>> +
>>>>
>>>> Sorry, if I am missing something, but do all devices supported by this
>>>> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an
>>>> error for a device not supporting PTM, would it?
>>>
>>> That was a very good question. I had to talk with the hardware folks.
>>> All the devices supported by the igc driver should support PTM.
>>
>> Thank you for checking that, that is valuable information.
>>
>>> And just to be clear, the reason that I am not returning an error here
>>> is that PTM could not be supported by the host system (think PCI
>>> controller).
>>
>> I just checked `pci_enable_ptm()` and on success it calls
>> `pci_ptm_info()` logging a message:
>>
>> 	pci_info(dev, "PTM enabled%s, %s granularity\n",
>> 		 dev->ptm_root ? " (root)" : "", clock_desc);
>>
>> Was that present on your system with your patch? Please add that to the
>> commit message.
> 
> Yes, with my patches applied I can see this message on my systems.
> 
> Sure, will add this to the commit message.
> 
>> Regarding my comment, I did not mean returning an error but the log
>> *level* of the message. So, `dmesg --level err` would show that message.
>> But if there are PCI controllers not supporting that, it’s not an error,
>> but a warning at most. So, I’d use:
>>
>> 	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller
>> (pci_enable_ptm() failed)\n");
> 
> I will use you suggestion for the message, but I think that warn is a
> bit too much, info or notice seem to be better.

I do not know, if modern PCI(e)(?) controllers normally support PTM or 
not. If recent controllers should support it, then a warning would be 
warranted, otherwise a notice.


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Vinicius Costa Gomes]

Hi Paul,

>> 
>>> Regarding my comment, I did not mean returning an error but the log
>>> *level* of the message. So, `dmesg --level err` would show that message.
>>> But if there are PCI controllers not supporting that, it’s not an error,
>>> but a warning at most. So, I’d use:
>>>
>>> 	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller
>>> (pci_enable_ptm() failed)\n");
>> 
>> I will use you suggestion for the message, but I think that warn is a
>> bit too much, info or notice seem to be better.
>
> I do not know, if modern PCI(e)(?) controllers normally support PTM or 
> not. If recent controllers should support it, then a warning would be 
> warranted, otherwise a notice.
>

From the Intel side, it seems that it's been supported for a few years.
So, fair enough, let's go with a warn.


Cheers,
-- 
Vinicius

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Bjorn Helgaas]

On Wed, Jun 09, 2021 at 04:07:20PM -0700, Vinicius Costa Gomes wrote:
> Hi Paul,
> 
> >> 
> >>> Regarding my comment, I did not mean returning an error but the log
> >>> *level* of the message. So, `dmesg --level err` would show that message.
> >>> But if there are PCI controllers not supporting that, it’s not an error,
> >>> but a warning at most. So, I’d use:
> >>>
> >>> 	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller
> >>> (pci_enable_ptm() failed)\n");
> >> 
> >> I will use you suggestion for the message, but I think that warn is a
> >> bit too much, info or notice seem to be better.
> >
> > I do not know, if modern PCI(e)(?) controllers normally support PTM or 
> > not. If recent controllers should support it, then a warning would be 
> > warranted, otherwise a notice.
> 
> From the Intel side, it seems that it's been supported for a few years.
> So, fair enough, let's go with a warn.

I'm not sure about this.  I think "warning" messages interrupt distro
graphical boot scenarios and cause user complaints.  In this case,
there is nothing broken and the user can do nothing about it; it's
merely a piece of missing optional functionality.  So I think "info"
is a more appropriate level.

Bjorn

Re: [Intel-wired-lan] [PATCH next-queue v5 3/4] igc: Enable PCIe PTM

[Vinicius Costa Gomes]

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Wed, Jun 09, 2021 at 04:07:20PM -0700, Vinicius Costa Gomes wrote:
>> Hi Paul,
>> 
>> >> 
>> >>> Regarding my comment, I did not mean returning an error but the log
>> >>> *level* of the message. So, `dmesg --level err` would show that message.
>> >>> But if there are PCI controllers not supporting that, it’s not an error,
>> >>> but a warning at most. So, I’d use:
>> >>>
>> >>> 	dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller
>> >>> (pci_enable_ptm() failed)\n");
>> >> 
>> >> I will use you suggestion for the message, but I think that warn is a
>> >> bit too much, info or notice seem to be better.
>> >
>> > I do not know, if modern PCI(e)(?) controllers normally support PTM or 
>> > not. If recent controllers should support it, then a warning would be 
>> > warranted, otherwise a notice.
>> 
>> From the Intel side, it seems that it's been supported for a few years.
>> So, fair enough, let's go with a warn.
>
> I'm not sure about this.  I think "warning" messages interrupt distro
> graphical boot scenarios and cause user complaints.  In this case,
> there is nothing broken and the user can do nothing about it; it's
> merely a piece of missing optional functionality.  So I think "info"
> is a more appropriate level.

Good point. "info" it is, then. 


Cheers,
-- 
Vinicius

[PATCH next-queue v5 4/4] igc: Add support for PTP getcrosststamp()

[Vinicius Costa Gomes]

i225 has support for PCIe PTM, which allows us to implement support
for the PTP_SYS_OFFSET_PRECISE ioctl(), implemented in the driver via
the getcrosststamp() function.

The easiest way to expose the PTM registers would be to configure the PTM
dialogs to run periodically, but the PTP_SYS_OFFSET_PRECISE ioctl()
semantics are more aligned to using a kind of "one-shot" way of retrieving
the PTM timestamps. But this causes a bit more code to be written: the
trigger registers for the PTM dialogs are not cleared automatically.

i225 can be configured to send "fake" packets with the PTM
information, adding support for handling these types of packets is
left for the future.

PTM improves the accuracy of time synchronization, for example, using
phc2sys. Before:

phc2sys[341.511]: CLOCK_REALTIME phc offset       289 s2 freq    +961 delay   2963
phc2sys[342.511]: CLOCK_REALTIME phc offset      -984 s2 freq    -225 delay   3517
phc2sys[343.511]: CLOCK_REALTIME phc offset       427 s2 freq    +891 delay   2312
phc2sys[344.511]: CLOCK_REALTIME phc offset       104 s2 freq    +696 delay   2575
phc2sys[345.511]: CLOCK_REALTIME phc offset       149 s2 freq    +772 delay   2388
phc2sys[346.511]: CLOCK_REALTIME phc offset        33 s2 freq    +701 delay   2359
phc2sys[347.511]: CLOCK_REALTIME phc offset      -216 s2 freq    +462 delay   2706
phc2sys[348.512]: CLOCK_REALTIME phc offset       140 s2 freq    +753 delay   2300
phc2sys[349.512]: CLOCK_REALTIME phc offset       -14 s2 freq    +641 delay   2385
phc2sys[350.512]: CLOCK_REALTIME phc offset      1048 s2 freq   +1699 delay   4303
phc2sys[351.512]: CLOCK_REALTIME phc offset     -1296 s2 freq    -331 delay   2846
phc2sys[352.512]: CLOCK_REALTIME phc offset      -912 s2 freq    -336 delay   4006
phc2sys[353.512]: CLOCK_REALTIME phc offset       880 s2 freq   +1183 delay   2338
phc2sys[354.512]: CLOCK_REALTIME phc offset       358 s2 freq    +925 delay   2348
phc2sys[355.512]: CLOCK_REALTIME phc offset      -211 s2 freq    +463 delay   2941
phc2sys[356.512]: CLOCK_REALTIME phc offset       234 s2 freq    +845 delay   2519
phc2sys[357.512]: CLOCK_REALTIME phc offset        45 s2 freq    +726 delay   2357
phc2sys[358.512]: CLOCK_REALTIME phc offset      -262 s2 freq    +433 delay   2821
phc2sys[359.512]: CLOCK_REALTIME phc offset      -424 s2 freq    +192 delay   3579
phc2sys[360.513]: CLOCK_REALTIME phc offset       134 s2 freq    +623 delay   3269
phc2sys[361.513]: CLOCK_REALTIME phc offset      -213 s2 freq    +316 delay   3999
phc2sys[362.513]: CLOCK_REALTIME phc offset      1023 s2 freq   +1488 delay   2614
phc2sys[363.513]: CLOCK_REALTIME phc offset        57 s2 freq    +829 delay   2332
phc2sys[364.513]: CLOCK_REALTIME phc offset      -126 s2 freq    +663 delay   2315
phc2sys[365.513]: CLOCK_REALTIME phc offset       -85 s2 freq    +666 delay   2449
phc2sys[366.513]: CLOCK_REALTIME phc offset      -193 s2 freq    +533 delay   2336
phc2sys[367.513]: CLOCK_REALTIME phc offset      -645 s2 freq     +23 delay   3870
phc2sys[368.513]: CLOCK_REALTIME phc offset       483 s2 freq    +957 delay   2342
phc2sys[369.513]: CLOCK_REALTIME phc offset      -166 s2 freq    +453 delay   3025
phc2sys[370.513]: CLOCK_REALTIME phc offset       327 s2 freq    +896 delay   2250

After:

phc2sys[617.838]: CLOCK_REALTIME phc offset       -25 s2 freq    +309 delay      0
phc2sys[618.838]: CLOCK_REALTIME phc offset       -43 s2 freq    +284 delay      0
phc2sys[619.838]: CLOCK_REALTIME phc offset       -12 s2 freq    +302 delay      0
phc2sys[620.838]: CLOCK_REALTIME phc offset        -2 s2 freq    +308 delay      0
phc2sys[621.838]: CLOCK_REALTIME phc offset        30 s2 freq    +340 delay      0
phc2sys[622.838]: CLOCK_REALTIME phc offset        14 s2 freq    +333 delay      0
phc2sys[623.839]: CLOCK_REALTIME phc offset        -3 s2 freq    +320 delay      0
phc2sys[624.839]: CLOCK_REALTIME phc offset         9 s2 freq    +331 delay      0
phc2sys[625.839]: CLOCK_REALTIME phc offset        -1 s2 freq    +324 delay      0
phc2sys[626.839]: CLOCK_REALTIME phc offset        -6 s2 freq    +318 delay      0
phc2sys[627.839]: CLOCK_REALTIME phc offset       -10 s2 freq    +313 delay      0
phc2sys[628.839]: CLOCK_REALTIME phc offset         7 s2 freq    +327 delay      0
phc2sys[629.839]: CLOCK_REALTIME phc offset         8 s2 freq    +330 delay      0
phc2sys[630.840]: CLOCK_REALTIME phc offset       -24 s2 freq    +300 delay      0
phc2sys[631.840]: CLOCK_REALTIME phc offset       -49 s2 freq    +268 delay      0
phc2sys[632.840]: CLOCK_REALTIME phc offset         6 s2 freq    +308 delay      0
phc2sys[633.840]: CLOCK_REALTIME phc offset        25 s2 freq    +329 delay      0
phc2sys[634.840]: CLOCK_REALTIME phc offset         5 s2 freq    +316 delay      0
phc2sys[635.840]: CLOCK_REALTIME phc offset        10 s2 freq    +323 delay      0
phc2sys[636.840]: CLOCK_REALTIME phc offset       -13 s2 freq    +303 delay      0
phc2sys[637.841]: CLOCK_REALTIME phc offset         4 s2 freq    +316 delay      0
phc2sys[638.841]: CLOCK_REALTIME phc offset        16 s2 freq    +329 delay      0
phc2sys[639.841]: CLOCK_REALTIME phc offset        31 s2 freq    +349 delay      0
phc2sys[640.841]: CLOCK_REALTIME phc offset       -21 s2 freq    +306 delay      0
phc2sys[641.841]: CLOCK_REALTIME phc offset       -14 s2 freq    +307 delay      0
phc2sys[642.841]: CLOCK_REALTIME phc offset       -24 s2 freq    +293 delay      0
phc2sys[643.841]: CLOCK_REALTIME phc offset        -6 s2 freq    +304 delay      0
phc2sys[644.842]: CLOCK_REALTIME phc offset        12 s2 freq    +320 delay      0
phc2sys[645.842]: CLOCK_REALTIME phc offset        12 s2 freq    +323 delay      0
phc2sys[646.842]: CLOCK_REALTIME phc offset       -12 s2 freq    +303 delay      0

One possible explanation is that when PTM is not enabled, and there's a lot
of traffic in the PCIe fabric, some register reads will take more time than
the others (see the variation in the delay values "before").

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |   1 +
 drivers/net/ethernet/intel/igc/igc_defines.h |  31 ++++
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 182 +++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  23 +++
 4 files changed, 237 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 5901ed9fb545..36ef4ba10e2c 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -225,6 +225,7 @@ struct igc_adapter {
 	struct timecounter tc;
 	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
 	ktime_t ptp_reset_start; /* Reset time in clock mono */
+	struct system_time_snapshot snapshot;
 
 	char fw_version[32];
 
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 71fe5b5ad2ed..0432ba26192e 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -481,6 +481,37 @@
 #define IGC_RXCSUM_CRCOFL	0x00000800   /* CRC32 offload enable */
 #define IGC_RXCSUM_PCSD		0x00002000   /* packet checksum disabled */
 
+/* PCIe PTM Control */
+#define IGC_PTM_CTRL_START_NOW	BIT(29) /* Start PTM Now */
+#define IGC_PTM_CTRL_EN		BIT(30) /* Enable PTM */
+#define IGC_PTM_CTRL_TRIG	BIT(31) /* PTM Cycle trigger */
+#define IGC_PTM_CTRL_SHRT_CYC(usec)	(((usec) & 0x2f) << 2)
+#define IGC_PTM_CTRL_PTM_TO(usec)	(((usec) & 0xff) << 8)
+
+#define IGC_PTM_SHORT_CYC_DEFAULT	10  /* Default Short/interrupted cycle interval */
+#define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
+#define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */
+
+/* PCIe Digital Delay */
+#define IGC_PCIE_DIG_DELAY_DEFAULT	0x01440000
+
+/* PCIe PHY Delay */
+#define IGC_PCIE_PHY_DELAY_DEFAULT	0x40900000
+
+#define IGC_TIMADJ_ADJUST_METH		0x40000000
+
+/* PCIe PTM Status */
+#define IGC_PTM_STAT_VALID		BIT(0) /* PTM Status */
+#define IGC_PTM_STAT_RET_ERR		BIT(1) /* Root port timeout */
+#define IGC_PTM_STAT_BAD_PTM_RES	BIT(2) /* PTM Response msg instead of PTM Response Data */
+#define IGC_PTM_STAT_T4M1_OVFL		BIT(3) /* T4 minus T1 overflow */
+#define IGC_PTM_STAT_ADJUST_1ST		BIT(4) /* 1588 timer adjusted during 1st PTM cycle */
+#define IGC_PTM_STAT_ADJUST_CYC		BIT(5) /* 1588 timer adjusted during non-1st PTM cycle */
+
+/* PCIe PTM Cycle Control */
+#define IGC_PTM_CYCLE_CTRL_CYC_TIME(msec)	((msec) & 0x3ff) /* PTM Cycle Time (msec) */
+#define IGC_PTM_CYCLE_CTRL_AUTO_CYC_EN		BIT(31) /* PTM Cycle Control */
+
 /* GPY211 - I225 defines */
 #define GPY_MMD_MASK		0xFFFF0000
 #define GPY_MMD_SHIFT		16
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 69617d2c1be2..1683b2f7cc8c 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -9,6 +9,8 @@
 #include <linux/ptp_classify.h>
 #include <linux/clocksource.h>
 #include <linux/ktime.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
 
 #define INCVALUE_MASK		0x7fffffff
 #define ISGN			0x80000000
@@ -16,6 +18,9 @@
 #define IGC_SYSTIM_OVERFLOW_PERIOD	(HZ * 60 * 9)
 #define IGC_PTP_TX_TIMEOUT		(HZ * 15)
 
+#define IGC_PTM_STAT_SLEEP		2
+#define IGC_PTM_STAT_TIMEOUT		100
+
 /* SYSTIM read access for I225 */
 void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts)
 {
@@ -752,6 +757,150 @@ int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr)
 		-EFAULT : 0;
 }
 
+/* Support for cross timestamping via PCIe PTM is only supported if
+ * two conditions are met:
+ *
+ * 1. We have an way to convert the timestamps in the PTM messages
+ *    to something related to the system clocks (right now, only
+ *    X86 systems with support for the Always Running Timer allow that);
+ *
+ * 2. We have PTM enabled in the path from the device to the PCIe root port.
+ */
+static bool igc_is_crosststamp_supported(struct igc_adapter *adapter)
+{
+#if IS_ENABLED(CONFIG_X86_TSC)
+	return pcie_ptm_enabled(adapter->pdev);
+#endif
+	return false;
+}
+
+static struct system_counterval_t igc_device_tstamp_to_system(u64 tstamp)
+{
+#if IS_ENABLED(CONFIG_X86_TSC)
+	return convert_art_ns_to_tsc(tstamp);
+#else
+	return (struct system_counterval_t) { };
+#endif
+}
+
+static void igc_ptm_log_error(struct igc_adapter *adapter, u32 ptm_stat)
+{
+	struct net_device *netdev = adapter->netdev;
+
+	switch (ptm_stat) {
+	case IGC_PTM_STAT_RET_ERR:
+		netdev_err(netdev, "PTM Error: Root port timeout\n");
+		break;
+	case IGC_PTM_STAT_BAD_PTM_RES:
+		netdev_err(netdev, "PTM Error: Bad response, PTM Response Data expected\n");
+		break;
+	case IGC_PTM_STAT_T4M1_OVFL:
+		netdev_err(netdev, "PTM Error: T4 minus T1 overflow\n");
+		break;
+	case IGC_PTM_STAT_ADJUST_1ST:
+		netdev_err(netdev, "PTM Error: 1588 timer adjusted during first PTM cycle\n");
+		break;
+	case IGC_PTM_STAT_ADJUST_CYC:
+		netdev_err(netdev, "PTM Error: 1588 timer adjusted during non-first PTM cycle\n");
+		break;
+	default:
+		netdev_err(netdev, "PTM Error: Unknown error (%#x)\n", ptm_stat);
+		break;
+	}
+}
+
+static int igc_phc_get_syncdevicetime(ktime_t *device,
+				      struct system_counterval_t *system,
+				      void *ctx)
+{
+	struct igc_adapter *adapter = ctx;
+	struct igc_hw *hw = &adapter->hw;
+	u32 stat, t2_curr_h, t2_curr_l, ctrl;
+	u32 t4mt1_prev, t3mt2_prev, delay;
+	ktime_t t1, t2_curr;
+	int err;
+
+	/* Get a snapshot of system clocks to use as historic value. */
+	ktime_get_snapshot(&adapter->snapshot);
+
+	do {
+		/* Doing this in a loop because in the event of a
+		 * badly timed (ha!) system clock adjustment, we may
+		 * get PTM Errors from the PCI root, but these errors
+		 * are transitory. Repeating the process returns valid
+		 * data eventually.
+		 */
+
+		/* To "manually" start the PTM cycle we need to clear and
+		 * then set again the TRIG bit.
+		 */
+		ctrl = rd32(IGC_PTM_CTRL);
+		ctrl &= ~IGC_PTM_CTRL_TRIG;
+		wr32(IGC_PTM_CTRL, ctrl);
+		ctrl |= IGC_PTM_CTRL_TRIG;
+		wr32(IGC_PTM_CTRL, ctrl);
+
+		/* The cycle only starts "for real" when software notifies
+		 * that it has read the registers, this is done by setting
+		 * VALID bit.
+		 */
+		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
+
+		err = readx_poll_timeout(rd32, IGC_PTM_STAT, stat,
+					 stat, IGC_PTM_STAT_SLEEP,
+					 IGC_PTM_STAT_TIMEOUT);
+		if (err < 0)
+			return err;
+
+		if ((stat & IGC_PTM_STAT_VALID) == IGC_PTM_STAT_VALID)
+			break;
+
+		if (stat & ~IGC_PTM_STAT_VALID) {
+			/* An error occurred, log it. */
+			igc_ptm_log_error(adapter, stat);
+			/* The STAT register is write-1-to-clear (W1C),
+			 * so write the previous error status to clear it.
+			 */
+			wr32(IGC_PTM_STAT, stat);
+			continue;
+		}
+	} while (true);
+
+	t1 = ktime_set(rd32(IGC_PTM_T1_TIM0_H),
+		       rd32(IGC_PTM_T1_TIM0_L));
+
+	t2_curr_l = rd32(IGC_PTM_CURR_T2_L);
+	t2_curr_h = rd32(IGC_PTM_CURR_T2_H);
+
+	/* FIXME: When the register that tells the endianness of the
+	 * PTM registers are implemented, check them here and add the
+	 * appropriate conversion.
+	 */
+	t2_curr_h = swab32(t2_curr_h);
+
+	t2_curr = ((s64)t2_curr_h << 32 | t2_curr_l);
+
+	t4mt1_prev = rd32(IGC_PTM_PREV_T4M1);
+	t3mt2_prev = rd32(IGC_PTM_PREV_T3M2);
+
+	delay = (t4mt1_prev - t3mt2_prev) / 2;
+
+	*device = t1 + delay;
+	*system = igc_device_tstamp_to_system(t2_curr);
+
+	return 0;
+}
+
+static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
+				  struct system_device_crosststamp *cts)
+{
+	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
+						   ptp_caps);
+
+	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
+					     adapter, &adapter->snapshot, cts);
+}
+
 /**
  * igc_ptp_init - Initialize PTP functionality
  * @adapter: Board private structure
@@ -788,6 +937,11 @@ void igc_ptp_init(struct igc_adapter *adapter)
 		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
 		adapter->ptp_caps.n_pins = IGC_N_SDP;
 		adapter->ptp_caps.verify = igc_ptp_verify_pin;
+
+		if (!igc_is_crosststamp_supported(adapter))
+			break;
+
+		adapter->ptp_caps.getcrosststamp = igc_ptp_getcrosststamp;
 		break;
 	default:
 		adapter->ptp_clock = NULL;
@@ -878,7 +1032,9 @@ void igc_ptp_stop(struct igc_adapter *adapter)
 void igc_ptp_reset(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
+	u32 cycle_ctrl, ctrl;
 	unsigned long flags;
+	u32 timadj;
 
 	/* reset the tstamp_config */
 	igc_ptp_set_timestamp_mode(adapter, &adapter->tstamp_config);
@@ -887,12 +1043,38 @@ void igc_ptp_reset(struct igc_adapter *adapter)
 
 	switch (adapter->hw.mac.type) {
 	case igc_i225:
+		timadj = rd32(IGC_TIMADJ);
+		timadj |= IGC_TIMADJ_ADJUST_METH;
+		wr32(IGC_TIMADJ, timadj);
+
 		wr32(IGC_TSAUXC, 0x0);
 		wr32(IGC_TSSDP, 0x0);
 		wr32(IGC_TSIM,
 		     IGC_TSICR_INTERRUPTS |
 		     (adapter->pps_sys_wrap_on ? IGC_TSICR_SYS_WRAP : 0));
 		wr32(IGC_IMS, IGC_IMS_TS);
+
+		if (!igc_is_crosststamp_supported(adapter))
+			break;
+
+		wr32(IGC_PCIE_DIG_DELAY, IGC_PCIE_DIG_DELAY_DEFAULT);
+		wr32(IGC_PCIE_PHY_DELAY, IGC_PCIE_PHY_DELAY_DEFAULT);
+
+		cycle_ctrl = IGC_PTM_CYCLE_CTRL_CYC_TIME(IGC_PTM_CYC_TIME_DEFAULT);
+
+		wr32(IGC_PTM_CYCLE_CTRL, cycle_ctrl);
+
+		ctrl = IGC_PTM_CTRL_EN |
+			IGC_PTM_CTRL_START_NOW |
+			IGC_PTM_CTRL_SHRT_CYC(IGC_PTM_SHORT_CYC_DEFAULT) |
+			IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT) |
+			IGC_PTM_CTRL_TRIG;
+
+		wr32(IGC_PTM_CTRL, ctrl);
+
+		/* Force the first cycle to run. */
+		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
+
 		break;
 	default:
 		/* No work to do. */
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 0f82990567d9..4499a6f7c577 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -229,6 +229,29 @@
 #define IGC_TXSTMPL	0x0B618  /* Tx timestamp value Low - RO */
 #define IGC_TXSTMPH	0x0B61C  /* Tx timestamp value High - RO */
 
+#define IGC_TIMADJ	0x0B60C  /* Time Adjustment Offset Register */
+
+/* PCIe Registers */
+#define IGC_PTM_CTRL		0x12540  /* PTM Control */
+#define IGC_PTM_STAT		0x12544  /* PTM Status */
+#define IGC_PTM_CYCLE_CTRL	0x1254C  /* PTM Cycle Control */
+
+/* PTM Time registers */
+#define IGC_PTM_T1_TIM0_L	0x12558  /* T1 on Timer 0 Low */
+#define IGC_PTM_T1_TIM0_H	0x1255C  /* T1 on Timer 0 High */
+
+#define IGC_PTM_CURR_T2_L	0x1258C  /* Current T2 Low */
+#define IGC_PTM_CURR_T2_H	0x12590  /* Current T2 High */
+#define IGC_PTM_PREV_T2_L	0x12584  /* Previous T2 Low */
+#define IGC_PTM_PREV_T2_H	0x12588  /* Previous T2 High */
+#define IGC_PTM_PREV_T4M1	0x12578  /* T4 Minus T1 on previous PTM Cycle */
+#define IGC_PTM_CURR_T4M1	0x1257C  /* T4 Minus T1 on this PTM Cycle */
+#define IGC_PTM_PREV_T3M2	0x12580  /* T3 Minus T2 on previous PTM Cycle */
+#define IGC_PTM_TDELAY		0x12594  /* PTM PCIe Link Delay */
+
+#define IGC_PCIE_DIG_DELAY	0x12550  /* PCIe Digital Delay */
+#define IGC_PCIE_PHY_DELAY	0x12554  /* PCIe PHY Delay */
+
 /* Management registers */
 #define IGC_MANC	0x05820  /* Management Control - RW */
 
-- 
2.31.1

Re: [Intel-wired-lan] [PATCH next-queue v5 4/4] igc: Add support for PTP getcrosststamp()

[Paul Menzel]

Dear Vinicius,


Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
> i225 has support for PCIe PTM, which allows us to implement support
> for the PTP_SYS_OFFSET_PRECISE ioctl(), implemented in the driver via
> the getcrosststamp() function.

Maybe:

i225 supports PCIe Precision Time Measurement (PTM), allowing us to 
support the PTP_SYS_OFFSET_PRECISE ioctl() in the driver via the 
getcrosststamp() function.

> The easiest way to expose the PTM registers would be to configure the PTM
> dialogs to run periodically, but the PTP_SYS_OFFSET_PRECISE ioctl()
> semantics are more aligned to using a kind of "one-shot" way of retrieving
> the PTM timestamps. But this causes a bit more code to be written: the

Maybe: But this results in more code:

> trigger registers for the PTM dialogs are not cleared automatically.
> 
> i225 can be configured to send "fake" packets with the PTM
> information, adding support for handling these types of packets is
> left for the future.
> 
> PTM improves the accuracy of time synchronization, for example, using
> phc2sys. Before:
> 
> phc2sys[341.511]: CLOCK_REALTIME phc offset       289 s2 freq    +961 delay   2963
> phc2sys[342.511]: CLOCK_REALTIME phc offset      -984 s2 freq    -225 delay   3517
> phc2sys[343.511]: CLOCK_REALTIME phc offset       427 s2 freq    +891 delay   2312
> phc2sys[344.511]: CLOCK_REALTIME phc offset       104 s2 freq    +696 delay   2575
> phc2sys[345.511]: CLOCK_REALTIME phc offset       149 s2 freq    +772 delay   2388
> phc2sys[346.511]: CLOCK_REALTIME phc offset        33 s2 freq    +701 delay   2359
> phc2sys[347.511]: CLOCK_REALTIME phc offset      -216 s2 freq    +462 delay   2706
> phc2sys[348.512]: CLOCK_REALTIME phc offset       140 s2 freq    +753 delay   2300
> phc2sys[349.512]: CLOCK_REALTIME phc offset       -14 s2 freq    +641 delay   2385
> phc2sys[350.512]: CLOCK_REALTIME phc offset      1048 s2 freq   +1699 delay   4303
> phc2sys[351.512]: CLOCK_REALTIME phc offset     -1296 s2 freq    -331 delay   2846
> phc2sys[352.512]: CLOCK_REALTIME phc offset      -912 s2 freq    -336 delay   4006
> phc2sys[353.512]: CLOCK_REALTIME phc offset       880 s2 freq   +1183 delay   2338
> phc2sys[354.512]: CLOCK_REALTIME phc offset       358 s2 freq    +925 delay   2348
> phc2sys[355.512]: CLOCK_REALTIME phc offset      -211 s2 freq    +463 delay   2941
> phc2sys[356.512]: CLOCK_REALTIME phc offset       234 s2 freq    +845 delay   2519
> phc2sys[357.512]: CLOCK_REALTIME phc offset        45 s2 freq    +726 delay   2357
> phc2sys[358.512]: CLOCK_REALTIME phc offset      -262 s2 freq    +433 delay   2821
> phc2sys[359.512]: CLOCK_REALTIME phc offset      -424 s2 freq    +192 delay   3579
> phc2sys[360.513]: CLOCK_REALTIME phc offset       134 s2 freq    +623 delay   3269
> phc2sys[361.513]: CLOCK_REALTIME phc offset      -213 s2 freq    +316 delay   3999
> phc2sys[362.513]: CLOCK_REALTIME phc offset      1023 s2 freq   +1488 delay   2614
> phc2sys[363.513]: CLOCK_REALTIME phc offset        57 s2 freq    +829 delay   2332
> phc2sys[364.513]: CLOCK_REALTIME phc offset      -126 s2 freq    +663 delay   2315
> phc2sys[365.513]: CLOCK_REALTIME phc offset       -85 s2 freq    +666 delay   2449
> phc2sys[366.513]: CLOCK_REALTIME phc offset      -193 s2 freq    +533 delay   2336
> phc2sys[367.513]: CLOCK_REALTIME phc offset      -645 s2 freq     +23 delay   3870
> phc2sys[368.513]: CLOCK_REALTIME phc offset       483 s2 freq    +957 delay   2342
> phc2sys[369.513]: CLOCK_REALTIME phc offset      -166 s2 freq    +453 delay   3025
> phc2sys[370.513]: CLOCK_REALTIME phc offset       327 s2 freq    +896 delay   2250
> 
> After:
> 
> phc2sys[617.838]: CLOCK_REALTIME phc offset       -25 s2 freq    +309 delay      0
> phc2sys[618.838]: CLOCK_REALTIME phc offset       -43 s2 freq    +284 delay      0
> phc2sys[619.838]: CLOCK_REALTIME phc offset       -12 s2 freq    +302 delay      0
> phc2sys[620.838]: CLOCK_REALTIME phc offset        -2 s2 freq    +308 delay      0
> phc2sys[621.838]: CLOCK_REALTIME phc offset        30 s2 freq    +340 delay      0
> phc2sys[622.838]: CLOCK_REALTIME phc offset        14 s2 freq    +333 delay      0
> phc2sys[623.839]: CLOCK_REALTIME phc offset        -3 s2 freq    +320 delay      0
> phc2sys[624.839]: CLOCK_REALTIME phc offset         9 s2 freq    +331 delay      0
> phc2sys[625.839]: CLOCK_REALTIME phc offset        -1 s2 freq    +324 delay      0
> phc2sys[626.839]: CLOCK_REALTIME phc offset        -6 s2 freq    +318 delay      0
> phc2sys[627.839]: CLOCK_REALTIME phc offset       -10 s2 freq    +313 delay      0
> phc2sys[628.839]: CLOCK_REALTIME phc offset         7 s2 freq    +327 delay      0
> phc2sys[629.839]: CLOCK_REALTIME phc offset         8 s2 freq    +330 delay      0
> phc2sys[630.840]: CLOCK_REALTIME phc offset       -24 s2 freq    +300 delay      0
> phc2sys[631.840]: CLOCK_REALTIME phc offset       -49 s2 freq    +268 delay      0
> phc2sys[632.840]: CLOCK_REALTIME phc offset         6 s2 freq    +308 delay      0
> phc2sys[633.840]: CLOCK_REALTIME phc offset        25 s2 freq    +329 delay      0
> phc2sys[634.840]: CLOCK_REALTIME phc offset         5 s2 freq    +316 delay      0
> phc2sys[635.840]: CLOCK_REALTIME phc offset        10 s2 freq    +323 delay      0
> phc2sys[636.840]: CLOCK_REALTIME phc offset       -13 s2 freq    +303 delay      0
> phc2sys[637.841]: CLOCK_REALTIME phc offset         4 s2 freq    +316 delay      0
> phc2sys[638.841]: CLOCK_REALTIME phc offset        16 s2 freq    +329 delay      0
> phc2sys[639.841]: CLOCK_REALTIME phc offset        31 s2 freq    +349 delay      0
> phc2sys[640.841]: CLOCK_REALTIME phc offset       -21 s2 freq    +306 delay      0
> phc2sys[641.841]: CLOCK_REALTIME phc offset       -14 s2 freq    +307 delay      0
> phc2sys[642.841]: CLOCK_REALTIME phc offset       -24 s2 freq    +293 delay      0
> phc2sys[643.841]: CLOCK_REALTIME phc offset        -6 s2 freq    +304 delay      0
> phc2sys[644.842]: CLOCK_REALTIME phc offset        12 s2 freq    +320 delay      0
> phc2sys[645.842]: CLOCK_REALTIME phc offset        12 s2 freq    +323 delay      0
> phc2sys[646.842]: CLOCK_REALTIME phc offset       -12 s2 freq    +303 delay      0

Please (additionally) summarize the findings by stating the min/max/avg?

> One possible explanation is that when PTM is not enabled, and there's a lot
> of traffic in the PCIe fabric, some register reads will take more time than
> the others (see the variation in the delay values "before").

Can you please document the datasheet name and revision used to 
implement this?

> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc.h         |   1 +
>   drivers/net/ethernet/intel/igc/igc_defines.h |  31 ++++
>   drivers/net/ethernet/intel/igc/igc_ptp.c     | 182 +++++++++++++++++++
>   drivers/net/ethernet/intel/igc/igc_regs.h    |  23 +++
>   4 files changed, 237 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 5901ed9fb545..36ef4ba10e2c 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -225,6 +225,7 @@ struct igc_adapter {
>   	struct timecounter tc;
>   	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
>   	ktime_t ptp_reset_start; /* Reset time in clock mono */
> +	struct system_time_snapshot snapshot;
>   
>   	char fw_version[32];
>   
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 71fe5b5ad2ed..0432ba26192e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -481,6 +481,37 @@
>   #define IGC_RXCSUM_CRCOFL	0x00000800   /* CRC32 offload enable */
>   #define IGC_RXCSUM_PCSD		0x00002000   /* packet checksum disabled */
>   
> +/* PCIe PTM Control */
> +#define IGC_PTM_CTRL_START_NOW	BIT(29) /* Start PTM Now */
> +#define IGC_PTM_CTRL_EN		BIT(30) /* Enable PTM */
> +#define IGC_PTM_CTRL_TRIG	BIT(31) /* PTM Cycle trigger */
> +#define IGC_PTM_CTRL_SHRT_CYC(usec)	(((usec) & 0x2f) << 2)
> +#define IGC_PTM_CTRL_PTM_TO(usec)	(((usec) & 0xff) << 8)
> +
> +#define IGC_PTM_SHORT_CYC_DEFAULT	10  /* Default Short/interrupted cycle interval */
> +#define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
> +#define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */
> +
> +/* PCIe Digital Delay */
> +#define IGC_PCIE_DIG_DELAY_DEFAULT	0x01440000
> +
> +/* PCIe PHY Delay */
> +#define IGC_PCIE_PHY_DELAY_DEFAULT	0x40900000
> +
> +#define IGC_TIMADJ_ADJUST_METH		0x40000000
> +
> +/* PCIe PTM Status */
> +#define IGC_PTM_STAT_VALID		BIT(0) /* PTM Status */
> +#define IGC_PTM_STAT_RET_ERR		BIT(1) /* Root port timeout */
> +#define IGC_PTM_STAT_BAD_PTM_RES	BIT(2) /* PTM Response msg instead of PTM Response Data */
> +#define IGC_PTM_STAT_T4M1_OVFL		BIT(3) /* T4 minus T1 overflow */
> +#define IGC_PTM_STAT_ADJUST_1ST		BIT(4) /* 1588 timer adjusted during 1st PTM cycle */
> +#define IGC_PTM_STAT_ADJUST_CYC		BIT(5) /* 1588 timer adjusted during non-1st PTM cycle */
> +
> +/* PCIe PTM Cycle Control */
> +#define IGC_PTM_CYCLE_CTRL_CYC_TIME(msec)	((msec) & 0x3ff) /* PTM Cycle Time (msec) */
> +#define IGC_PTM_CYCLE_CTRL_AUTO_CYC_EN		BIT(31) /* PTM Cycle Control */
> +
>   /* GPY211 - I225 defines */
>   #define GPY_MMD_MASK		0xFFFF0000
>   #define GPY_MMD_SHIFT		16
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 69617d2c1be2..1683b2f7cc8c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -9,6 +9,8 @@
>   #include <linux/ptp_classify.h>
>   #include <linux/clocksource.h>
>   #include <linux/ktime.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
>   
>   #define INCVALUE_MASK		0x7fffffff
>   #define ISGN			0x80000000
> @@ -16,6 +18,9 @@
>   #define IGC_SYSTIM_OVERFLOW_PERIOD	(HZ * 60 * 9)
>   #define IGC_PTP_TX_TIMEOUT		(HZ * 15)
>   
> +#define IGC_PTM_STAT_SLEEP		2
> +#define IGC_PTM_STAT_TIMEOUT		100
> +
>   /* SYSTIM read access for I225 */
>   void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts)
>   {
> @@ -752,6 +757,150 @@ int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr)
>   		-EFAULT : 0;
>   }
>   
> +/* Support for cross timestamping via PCIe PTM is only supported if
> + * two conditions are met:

Maybe: The two conditions below must be met for cross timestamping via 
PCIe PTM

> + *
> + * 1. We have an way to convert the timestamps in the PTM messages

s/an way/a way/

> + *    to something related to the system clocks (right now, only
> + *    X86 systems with support for the Always Running Timer allow that);
> + *
> + * 2. We have PTM enabled in the path from the device to the PCIe root port.
> + */
> +static bool igc_is_crosststamp_supported(struct igc_adapter *adapter)
> +{
> +#if IS_ENABLED(CONFIG_X86_TSC)
> +	return pcie_ptm_enabled(adapter->pdev);
> +#endif
> +	return false;

I’d also add the preprocessor else branch as below (despite the compiler 
opitimzing it away) for readability. Also, I’d do the check in C and not 
the preprocessor.

     return IS_ENABLED(CONFIG_X86_TSC) ? pcie_ptm_enabled(adapter->pdev) 
: false;

> +}
> +
> +static struct system_counterval_t igc_device_tstamp_to_system(u64 tstamp)
> +{
> +#if IS_ENABLED(CONFIG_X86_TSC)
> +	return convert_art_ns_to_tsc(tstamp);
> +#else
> +	return (struct system_counterval_t) { };
> +#endif
> +}
> +
> +static void igc_ptm_log_error(struct igc_adapter *adapter, u32 ptm_stat)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +
> +	switch (ptm_stat) {
> +	case IGC_PTM_STAT_RET_ERR:
> +		netdev_err(netdev, "PTM Error: Root port timeout\n");
> +		break;
> +	case IGC_PTM_STAT_BAD_PTM_RES:
> +		netdev_err(netdev, "PTM Error: Bad response, PTM Response Data expected\n");
> +		break;
> +	case IGC_PTM_STAT_T4M1_OVFL:
> +		netdev_err(netdev, "PTM Error: T4 minus T1 overflow\n");
> +		break;
> +	case IGC_PTM_STAT_ADJUST_1ST:
> +		netdev_err(netdev, "PTM Error: 1588 timer adjusted during first PTM cycle\n");
> +		break;
> +	case IGC_PTM_STAT_ADJUST_CYC:
> +		netdev_err(netdev, "PTM Error: 1588 timer adjusted during non-first PTM cycle\n");
> +		break;
> +	default:
> +		netdev_err(netdev, "PTM Error: Unknown error (%#x)\n", ptm_stat);
> +		break;
> +	}
> +}
> +
> +static int igc_phc_get_syncdevicetime(ktime_t *device,
> +				      struct system_counterval_t *system,
> +				      void *ctx)
> +{
> +	struct igc_adapter *adapter = ctx;
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 stat, t2_curr_h, t2_curr_l, ctrl;
> +	u32 t4mt1_prev, t3mt2_prev, delay;
> +	ktime_t t1, t2_curr;
> +	int err;
> +
> +	/* Get a snapshot of system clocks to use as historic value. */
> +	ktime_get_snapshot(&adapter->snapshot);
> +
> +	do {
> +		/* Doing this in a loop because in the event of a
> +		 * badly timed (ha!) system clock adjustment, we may
> +		 * get PTM Errors from the PCI root, but these errors

PTM errors

> +		 * are transitory. Repeating the process returns valid
> +		 * data eventually.
> +		 */
> +
> +		/* To "manually" start the PTM cycle we need to clear and
> +		 * then set again the TRIG bit.
> +		 */
> +		ctrl = rd32(IGC_PTM_CTRL);
> +		ctrl &= ~IGC_PTM_CTRL_TRIG;
> +		wr32(IGC_PTM_CTRL, ctrl);
> +		ctrl |= IGC_PTM_CTRL_TRIG;
> +		wr32(IGC_PTM_CTRL, ctrl);
> +
> +		/* The cycle only starts "for real" when software notifies
> +		 * that it has read the registers, this is done by setting
> +		 * VALID bit.
> +		 */
> +		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
> +
> +		err = readx_poll_timeout(rd32, IGC_PTM_STAT, stat,
> +					 stat, IGC_PTM_STAT_SLEEP,
> +					 IGC_PTM_STAT_TIMEOUT);
> +		if (err < 0)
> +			return err;

Should this be logged?

> +
> +		if ((stat & IGC_PTM_STAT_VALID) == IGC_PTM_STAT_VALID)
> +			break;
> +
> +		if (stat & ~IGC_PTM_STAT_VALID) {
> +			/* An error occurred, log it. */
> +			igc_ptm_log_error(adapter, stat);
> +			/* The STAT register is write-1-to-clear (W1C),
> +			 * so write the previous error status to clear it.
> +			 */
> +			wr32(IGC_PTM_STAT, stat);
> +			continue;
> +		}
> +	} while (true);

I personally prefer to write at least one condition in the loop condition.

> +
> +	t1 = ktime_set(rd32(IGC_PTM_T1_TIM0_H),
> +		       rd32(IGC_PTM_T1_TIM0_L));

Why not put it into one line?

> +
> +	t2_curr_l = rd32(IGC_PTM_CURR_T2_L);
> +	t2_curr_h = rd32(IGC_PTM_CURR_T2_H);
> +
> +	/* FIXME: When the register that tells the endianness of the
> +	 * PTM registers are implemented, check them here and add the
> +	 * appropriate conversion.
> +	 */
> +	t2_curr_h = swab32(t2_curr_h);
> +
> +	t2_curr = ((s64)t2_curr_h << 32 | t2_curr_l);
> +
> +	t4mt1_prev = rd32(IGC_PTM_PREV_T4M1);
> +	t3mt2_prev = rd32(IGC_PTM_PREV_T3M2);
> +
> +	delay = (t4mt1_prev - t3mt2_prev) / 2;
> +
> +	*device = t1 + delay;
> +	*system = igc_device_tstamp_to_system(t2_curr);
> +
> +	return 0;
> +}
> +
> +static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
> +				  struct system_device_crosststamp *cts)
> +{
> +	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
> +						   ptp_caps);
> +
> +	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
> +					     adapter, &adapter->snapshot, cts);
> +}
> +
>   /**
>    * igc_ptp_init - Initialize PTP functionality
>    * @adapter: Board private structure
> @@ -788,6 +937,11 @@ void igc_ptp_init(struct igc_adapter *adapter)
>   		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
>   		adapter->ptp_caps.n_pins = IGC_N_SDP;
>   		adapter->ptp_caps.verify = igc_ptp_verify_pin;
> +
> +		if (!igc_is_crosststamp_supported(adapter))
> +			break;
> +
> +		adapter->ptp_caps.getcrosststamp = igc_ptp_getcrosststamp;
>   		break;
>   	default:
>   		adapter->ptp_clock = NULL;
> @@ -878,7 +1032,9 @@ void igc_ptp_stop(struct igc_adapter *adapter)
>   void igc_ptp_reset(struct igc_adapter *adapter)
>   {
>   	struct igc_hw *hw = &adapter->hw;
> +	u32 cycle_ctrl, ctrl;
>   	unsigned long flags;
> +	u32 timadj;
>   
>   	/* reset the tstamp_config */
>   	igc_ptp_set_timestamp_mode(adapter, &adapter->tstamp_config);
> @@ -887,12 +1043,38 @@ void igc_ptp_reset(struct igc_adapter *adapter)
>   
>   	switch (adapter->hw.mac.type) {
>   	case igc_i225:
> +		timadj = rd32(IGC_TIMADJ);
> +		timadj |= IGC_TIMADJ_ADJUST_METH;
> +		wr32(IGC_TIMADJ, timadj);
> +
>   		wr32(IGC_TSAUXC, 0x0);
>   		wr32(IGC_TSSDP, 0x0);
>   		wr32(IGC_TSIM,
>   		     IGC_TSICR_INTERRUPTS |
>   		     (adapter->pps_sys_wrap_on ? IGC_TSICR_SYS_WRAP : 0));
>   		wr32(IGC_IMS, IGC_IMS_TS);
> +
> +		if (!igc_is_crosststamp_supported(adapter))
> +			break;
> +
> +		wr32(IGC_PCIE_DIG_DELAY, IGC_PCIE_DIG_DELAY_DEFAULT);
> +		wr32(IGC_PCIE_PHY_DELAY, IGC_PCIE_PHY_DELAY_DEFAULT);
> +
> +		cycle_ctrl = IGC_PTM_CYCLE_CTRL_CYC_TIME(IGC_PTM_CYC_TIME_DEFAULT);
> +
> +		wr32(IGC_PTM_CYCLE_CTRL, cycle_ctrl);
> +
> +		ctrl = IGC_PTM_CTRL_EN |
> +			IGC_PTM_CTRL_START_NOW |
> +			IGC_PTM_CTRL_SHRT_CYC(IGC_PTM_SHORT_CYC_DEFAULT) |
> +			IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT) |
> +			IGC_PTM_CTRL_TRIG;
> +
> +		wr32(IGC_PTM_CTRL, ctrl);
> +
> +		/* Force the first cycle to run. */
> +		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
> +
>   		break;
>   	default:
>   		/* No work to do. */
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index 0f82990567d9..4499a6f7c577 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -229,6 +229,29 @@
>   #define IGC_TXSTMPL	0x0B618  /* Tx timestamp value Low - RO */
>   #define IGC_TXSTMPH	0x0B61C  /* Tx timestamp value High - RO */
>   
> +#define IGC_TIMADJ	0x0B60C  /* Time Adjustment Offset Register */
> +
> +/* PCIe Registers */
> +#define IGC_PTM_CTRL		0x12540  /* PTM Control */
> +#define IGC_PTM_STAT		0x12544  /* PTM Status */
> +#define IGC_PTM_CYCLE_CTRL	0x1254C  /* PTM Cycle Control */
> +
> +/* PTM Time registers */
> +#define IGC_PTM_T1_TIM0_L	0x12558  /* T1 on Timer 0 Low */
> +#define IGC_PTM_T1_TIM0_H	0x1255C  /* T1 on Timer 0 High */
> +
> +#define IGC_PTM_CURR_T2_L	0x1258C  /* Current T2 Low */
> +#define IGC_PTM_CURR_T2_H	0x12590  /* Current T2 High */
> +#define IGC_PTM_PREV_T2_L	0x12584  /* Previous T2 Low */
> +#define IGC_PTM_PREV_T2_H	0x12588  /* Previous T2 High */
> +#define IGC_PTM_PREV_T4M1	0x12578  /* T4 Minus T1 on previous PTM Cycle */
> +#define IGC_PTM_CURR_T4M1	0x1257C  /* T4 Minus T1 on this PTM Cycle */
> +#define IGC_PTM_PREV_T3M2	0x12580  /* T3 Minus T2 on previous PTM Cycle */
> +#define IGC_PTM_TDELAY		0x12594  /* PTM PCIe Link Delay */
> +
> +#define IGC_PCIE_DIG_DELAY	0x12550  /* PCIe Digital Delay */
> +#define IGC_PCIE_PHY_DELAY	0x12554  /* PCIe PHY Delay */
> +
>   /* Management registers */
>   #define IGC_MANC	0x05820  /* Management Control - RW */
>   

How can the user find out, that PTP grecroosstamp() is used?


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH next-queue v5 4/4] igc: Add support for PTP getcrosststamp()

[Vinicius Costa Gomes]

Paul Menzel <pmenzel@molgen.mpg.de> writes:

> Dear Vinicius,
>
>
> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes:
>> i225 has support for PCIe PTM, which allows us to implement support
>> for the PTP_SYS_OFFSET_PRECISE ioctl(), implemented in the driver via
>> the getcrosststamp() function.
>
> Maybe:
>
> i225 supports PCIe Precision Time Measurement (PTM), allowing us to 
> support the PTP_SYS_OFFSET_PRECISE ioctl() in the driver via the 
> getcrosststamp() function.

Sure. Sounds good. Will change the commit message.

>
>> The easiest way to expose the PTM registers would be to configure the PTM
>> dialogs to run periodically, but the PTP_SYS_OFFSET_PRECISE ioctl()
>> semantics are more aligned to using a kind of "one-shot" way of retrieving
>> the PTM timestamps. But this causes a bit more code to be written: the
>
> Maybe: But this results in more code:

Will change.

>
>> trigger registers for the PTM dialogs are not cleared automatically.
>> 
>> i225 can be configured to send "fake" packets with the PTM
>> information, adding support for handling these types of packets is
>> left for the future.
>> 
>> PTM improves the accuracy of time synchronization, for example, using
>> phc2sys. Before:
>> 
>> phc2sys[341.511]: CLOCK_REALTIME phc offset       289 s2 freq    +961 delay   2963
>> phc2sys[342.511]: CLOCK_REALTIME phc offset      -984 s2 freq    -225 delay   3517
>> phc2sys[343.511]: CLOCK_REALTIME phc offset       427 s2 freq    +891 delay   2312
>> phc2sys[344.511]: CLOCK_REALTIME phc offset       104 s2 freq    +696 delay   2575
>> phc2sys[345.511]: CLOCK_REALTIME phc offset       149 s2 freq    +772 delay   2388
>> phc2sys[346.511]: CLOCK_REALTIME phc offset        33 s2 freq    +701 delay   2359
>> phc2sys[347.511]: CLOCK_REALTIME phc offset      -216 s2 freq    +462 delay   2706
>> phc2sys[348.512]: CLOCK_REALTIME phc offset       140 s2 freq    +753 delay   2300
>> phc2sys[349.512]: CLOCK_REALTIME phc offset       -14 s2 freq    +641 delay   2385
>> phc2sys[350.512]: CLOCK_REALTIME phc offset      1048 s2 freq   +1699 delay   4303
>> phc2sys[351.512]: CLOCK_REALTIME phc offset     -1296 s2 freq    -331 delay   2846
>> phc2sys[352.512]: CLOCK_REALTIME phc offset      -912 s2 freq    -336 delay   4006
>> phc2sys[353.512]: CLOCK_REALTIME phc offset       880 s2 freq   +1183 delay   2338
>> phc2sys[354.512]: CLOCK_REALTIME phc offset       358 s2 freq    +925 delay   2348
>> phc2sys[355.512]: CLOCK_REALTIME phc offset      -211 s2 freq    +463 delay   2941
>> phc2sys[356.512]: CLOCK_REALTIME phc offset       234 s2 freq    +845 delay   2519
>> phc2sys[357.512]: CLOCK_REALTIME phc offset        45 s2 freq    +726 delay   2357
>> phc2sys[358.512]: CLOCK_REALTIME phc offset      -262 s2 freq    +433 delay   2821
>> phc2sys[359.512]: CLOCK_REALTIME phc offset      -424 s2 freq    +192 delay   3579
>> phc2sys[360.513]: CLOCK_REALTIME phc offset       134 s2 freq    +623 delay   3269
>> phc2sys[361.513]: CLOCK_REALTIME phc offset      -213 s2 freq    +316 delay   3999
>> phc2sys[362.513]: CLOCK_REALTIME phc offset      1023 s2 freq   +1488 delay   2614
>> phc2sys[363.513]: CLOCK_REALTIME phc offset        57 s2 freq    +829 delay   2332
>> phc2sys[364.513]: CLOCK_REALTIME phc offset      -126 s2 freq    +663 delay   2315
>> phc2sys[365.513]: CLOCK_REALTIME phc offset       -85 s2 freq    +666 delay   2449
>> phc2sys[366.513]: CLOCK_REALTIME phc offset      -193 s2 freq    +533 delay   2336
>> phc2sys[367.513]: CLOCK_REALTIME phc offset      -645 s2 freq     +23 delay   3870
>> phc2sys[368.513]: CLOCK_REALTIME phc offset       483 s2 freq    +957 delay   2342
>> phc2sys[369.513]: CLOCK_REALTIME phc offset      -166 s2 freq    +453 delay   3025
>> phc2sys[370.513]: CLOCK_REALTIME phc offset       327 s2 freq    +896 delay   2250
>> 
>> After:
>> 
>> phc2sys[617.838]: CLOCK_REALTIME phc offset       -25 s2 freq    +309 delay      0
>> phc2sys[618.838]: CLOCK_REALTIME phc offset       -43 s2 freq    +284 delay      0
>> phc2sys[619.838]: CLOCK_REALTIME phc offset       -12 s2 freq    +302 delay      0
>> phc2sys[620.838]: CLOCK_REALTIME phc offset        -2 s2 freq    +308 delay      0
>> phc2sys[621.838]: CLOCK_REALTIME phc offset        30 s2 freq    +340 delay      0
>> phc2sys[622.838]: CLOCK_REALTIME phc offset        14 s2 freq    +333 delay      0
>> phc2sys[623.839]: CLOCK_REALTIME phc offset        -3 s2 freq    +320 delay      0
>> phc2sys[624.839]: CLOCK_REALTIME phc offset         9 s2 freq    +331 delay      0
>> phc2sys[625.839]: CLOCK_REALTIME phc offset        -1 s2 freq    +324 delay      0
>> phc2sys[626.839]: CLOCK_REALTIME phc offset        -6 s2 freq    +318 delay      0
>> phc2sys[627.839]: CLOCK_REALTIME phc offset       -10 s2 freq    +313 delay      0
>> phc2sys[628.839]: CLOCK_REALTIME phc offset         7 s2 freq    +327 delay      0
>> phc2sys[629.839]: CLOCK_REALTIME phc offset         8 s2 freq    +330 delay      0
>> phc2sys[630.840]: CLOCK_REALTIME phc offset       -24 s2 freq    +300 delay      0
>> phc2sys[631.840]: CLOCK_REALTIME phc offset       -49 s2 freq    +268 delay      0
>> phc2sys[632.840]: CLOCK_REALTIME phc offset         6 s2 freq    +308 delay      0
>> phc2sys[633.840]: CLOCK_REALTIME phc offset        25 s2 freq    +329 delay      0
>> phc2sys[634.840]: CLOCK_REALTIME phc offset         5 s2 freq    +316 delay      0
>> phc2sys[635.840]: CLOCK_REALTIME phc offset        10 s2 freq    +323 delay      0
>> phc2sys[636.840]: CLOCK_REALTIME phc offset       -13 s2 freq    +303 delay      0
>> phc2sys[637.841]: CLOCK_REALTIME phc offset         4 s2 freq    +316 delay      0
>> phc2sys[638.841]: CLOCK_REALTIME phc offset        16 s2 freq    +329 delay      0
>> phc2sys[639.841]: CLOCK_REALTIME phc offset        31 s2 freq    +349 delay      0
>> phc2sys[640.841]: CLOCK_REALTIME phc offset       -21 s2 freq    +306 delay      0
>> phc2sys[641.841]: CLOCK_REALTIME phc offset       -14 s2 freq    +307 delay      0
>> phc2sys[642.841]: CLOCK_REALTIME phc offset       -24 s2 freq    +293 delay      0
>> phc2sys[643.841]: CLOCK_REALTIME phc offset        -6 s2 freq    +304 delay      0
>> phc2sys[644.842]: CLOCK_REALTIME phc offset        12 s2 freq    +320 delay      0
>> phc2sys[645.842]: CLOCK_REALTIME phc offset        12 s2 freq    +323 delay      0
>> phc2sys[646.842]: CLOCK_REALTIME phc offset       -12 s2 freq    +303 delay      0
>
> Please (additionally) summarize the findings by stating the
> min/max/avg?

Sure. Will do.

>
>> One possible explanation is that when PTM is not enabled, and there's a lot
>> of traffic in the PCIe fabric, some register reads will take more time than
>> the others (see the variation in the delay values "before").
>
> Can you please document the datasheet name and revision used to 
> implement this?

Sure. Will document this for the next version.

>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc.h         |   1 +
>>   drivers/net/ethernet/intel/igc/igc_defines.h |  31 ++++
>>   drivers/net/ethernet/intel/igc/igc_ptp.c     | 182 +++++++++++++++++++
>>   drivers/net/ethernet/intel/igc/igc_regs.h    |  23 +++
>>   4 files changed, 237 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index 5901ed9fb545..36ef4ba10e2c 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -225,6 +225,7 @@ struct igc_adapter {
>>   	struct timecounter tc;
>>   	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
>>   	ktime_t ptp_reset_start; /* Reset time in clock mono */
>> +	struct system_time_snapshot snapshot;
>>   
>>   	char fw_version[32];
>>   
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 71fe5b5ad2ed..0432ba26192e 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -481,6 +481,37 @@
>>   #define IGC_RXCSUM_CRCOFL	0x00000800   /* CRC32 offload enable */
>>   #define IGC_RXCSUM_PCSD		0x00002000   /* packet checksum disabled */
>>   
>> +/* PCIe PTM Control */
>> +#define IGC_PTM_CTRL_START_NOW	BIT(29) /* Start PTM Now */
>> +#define IGC_PTM_CTRL_EN		BIT(30) /* Enable PTM */
>> +#define IGC_PTM_CTRL_TRIG	BIT(31) /* PTM Cycle trigger */
>> +#define IGC_PTM_CTRL_SHRT_CYC(usec)	(((usec) & 0x2f) << 2)
>> +#define IGC_PTM_CTRL_PTM_TO(usec)	(((usec) & 0xff) << 8)
>> +
>> +#define IGC_PTM_SHORT_CYC_DEFAULT	10  /* Default Short/interrupted cycle interval */
>> +#define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
>> +#define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */
>> +
>> +/* PCIe Digital Delay */
>> +#define IGC_PCIE_DIG_DELAY_DEFAULT	0x01440000
>> +
>> +/* PCIe PHY Delay */
>> +#define IGC_PCIE_PHY_DELAY_DEFAULT	0x40900000
>> +
>> +#define IGC_TIMADJ_ADJUST_METH		0x40000000
>> +
>> +/* PCIe PTM Status */
>> +#define IGC_PTM_STAT_VALID		BIT(0) /* PTM Status */
>> +#define IGC_PTM_STAT_RET_ERR		BIT(1) /* Root port timeout */
>> +#define IGC_PTM_STAT_BAD_PTM_RES	BIT(2) /* PTM Response msg instead of PTM Response Data */
>> +#define IGC_PTM_STAT_T4M1_OVFL		BIT(3) /* T4 minus T1 overflow */
>> +#define IGC_PTM_STAT_ADJUST_1ST		BIT(4) /* 1588 timer adjusted during 1st PTM cycle */
>> +#define IGC_PTM_STAT_ADJUST_CYC		BIT(5) /* 1588 timer adjusted during non-1st PTM cycle */
>> +
>> +/* PCIe PTM Cycle Control */
>> +#define IGC_PTM_CYCLE_CTRL_CYC_TIME(msec)	((msec) & 0x3ff) /* PTM Cycle Time (msec) */
>> +#define IGC_PTM_CYCLE_CTRL_AUTO_CYC_EN		BIT(31) /* PTM Cycle Control */
>> +
>>   /* GPY211 - I225 defines */
>>   #define GPY_MMD_MASK		0xFFFF0000
>>   #define GPY_MMD_SHIFT		16
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> index 69617d2c1be2..1683b2f7cc8c 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> @@ -9,6 +9,8 @@
>>   #include <linux/ptp_classify.h>
>>   #include <linux/clocksource.h>
>>   #include <linux/ktime.h>
>> +#include <linux/delay.h>
>> +#include <linux/iopoll.h>
>>   
>>   #define INCVALUE_MASK		0x7fffffff
>>   #define ISGN			0x80000000
>> @@ -16,6 +18,9 @@
>>   #define IGC_SYSTIM_OVERFLOW_PERIOD	(HZ * 60 * 9)
>>   #define IGC_PTP_TX_TIMEOUT		(HZ * 15)
>>   
>> +#define IGC_PTM_STAT_SLEEP		2
>> +#define IGC_PTM_STAT_TIMEOUT		100
>> +
>>   /* SYSTIM read access for I225 */
>>   void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts)
>>   {
>> @@ -752,6 +757,150 @@ int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr)
>>   		-EFAULT : 0;
>>   }
>>   
>> +/* Support for cross timestamping via PCIe PTM is only supported if
>> + * two conditions are met:
>
> Maybe: The two conditions below must be met for cross timestamping via 
> PCIe PTM
>

Will use your suggestion.

>> + *
>> + * 1. We have an way to convert the timestamps in the PTM messages
>
> s/an way/a way/

Will fix.

>
>> + *    to something related to the system clocks (right now, only
>> + *    X86 systems with support for the Always Running Timer allow that);
>> + *
>> + * 2. We have PTM enabled in the path from the device to the PCIe root port.
>> + */
>> +static bool igc_is_crosststamp_supported(struct igc_adapter *adapter)
>> +{
>> +#if IS_ENABLED(CONFIG_X86_TSC)
>> +	return pcie_ptm_enabled(adapter->pdev);
>> +#endif
>> +	return false;
>
> I’d also add the preprocessor else branch as below (despite the compiler 
> opitimzing it away) for readability. Also, I’d do the check in C and not 
> the preprocessor.
>
>      return IS_ENABLED(CONFIG_X86_TSC) ? pcie_ptm_enabled(adapter->pdev) 
> : false;
>

I was using the preprocessor because, before, there was a check for
CONFIG_PCIE_PTM, now that that check doesn't exist anymore. I can do it
in C. Will change. Thanks.

>> +}
>> +
>> +static struct system_counterval_t igc_device_tstamp_to_system(u64 tstamp)
>> +{
>> +#if IS_ENABLED(CONFIG_X86_TSC)
>> +	return convert_art_ns_to_tsc(tstamp);
>> +#else
>> +	return (struct system_counterval_t) { };
>> +#endif
>> +}
>> +
>> +static void igc_ptm_log_error(struct igc_adapter *adapter, u32 ptm_stat)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +
>> +	switch (ptm_stat) {
>> +	case IGC_PTM_STAT_RET_ERR:
>> +		netdev_err(netdev, "PTM Error: Root port timeout\n");
>> +		break;
>> +	case IGC_PTM_STAT_BAD_PTM_RES:
>> +		netdev_err(netdev, "PTM Error: Bad response, PTM Response Data expected\n");
>> +		break;
>> +	case IGC_PTM_STAT_T4M1_OVFL:
>> +		netdev_err(netdev, "PTM Error: T4 minus T1 overflow\n");
>> +		break;
>> +	case IGC_PTM_STAT_ADJUST_1ST:
>> +		netdev_err(netdev, "PTM Error: 1588 timer adjusted during first PTM cycle\n");
>> +		break;
>> +	case IGC_PTM_STAT_ADJUST_CYC:
>> +		netdev_err(netdev, "PTM Error: 1588 timer adjusted during non-first PTM cycle\n");
>> +		break;
>> +	default:
>> +		netdev_err(netdev, "PTM Error: Unknown error (%#x)\n", ptm_stat);
>> +		break;
>> +	}
>> +}
>> +
>> +static int igc_phc_get_syncdevicetime(ktime_t *device,
>> +				      struct system_counterval_t *system,
>> +				      void *ctx)
>> +{
>> +	struct igc_adapter *adapter = ctx;
>> +	struct igc_hw *hw = &adapter->hw;
>> +	u32 stat, t2_curr_h, t2_curr_l, ctrl;
>> +	u32 t4mt1_prev, t3mt2_prev, delay;
>> +	ktime_t t1, t2_curr;
>> +	int err;
>> +
>> +	/* Get a snapshot of system clocks to use as historic value. */
>> +	ktime_get_snapshot(&adapter->snapshot);
>> +
>> +	do {
>> +		/* Doing this in a loop because in the event of a
>> +		 * badly timed (ha!) system clock adjustment, we may
>> +		 * get PTM Errors from the PCI root, but these errors
>
> PTM errors

Will fix. Thanks.

>
>> +		 * are transitory. Repeating the process returns valid
>> +		 * data eventually.
>> +		 */
>> +
>> +		/* To "manually" start the PTM cycle we need to clear and
>> +		 * then set again the TRIG bit.
>> +		 */
>> +		ctrl = rd32(IGC_PTM_CTRL);
>> +		ctrl &= ~IGC_PTM_CTRL_TRIG;
>> +		wr32(IGC_PTM_CTRL, ctrl);
>> +		ctrl |= IGC_PTM_CTRL_TRIG;
>> +		wr32(IGC_PTM_CTRL, ctrl);
>> +
>> +		/* The cycle only starts "for real" when software notifies
>> +		 * that it has read the registers, this is done by setting
>> +		 * VALID bit.
>> +		 */
>> +		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
>> +
>> +		err = readx_poll_timeout(rd32, IGC_PTM_STAT, stat,
>> +					 stat, IGC_PTM_STAT_SLEEP,
>> +					 IGC_PTM_STAT_TIMEOUT);
>> +		if (err < 0)
>> +			return err;
>
> Should this be logged?

Good catch. Will fix.

>
>> +
>> +		if ((stat & IGC_PTM_STAT_VALID) == IGC_PTM_STAT_VALID)
>> +			break;
>> +
>> +		if (stat & ~IGC_PTM_STAT_VALID) {
>> +			/* An error occurred, log it. */
>> +			igc_ptm_log_error(adapter, stat);
>> +			/* The STAT register is write-1-to-clear (W1C),
>> +			 * so write the previous error status to clear it.
>> +			 */
>> +			wr32(IGC_PTM_STAT, stat);
>> +			continue;
>> +		}
>> +	} while (true);
>
> I personally prefer to write at least one condition in the loop
> condition.

I can do that. Will fix.

>
>> +
>> +	t1 = ktime_set(rd32(IGC_PTM_T1_TIM0_H),
>> +		       rd32(IGC_PTM_T1_TIM0_L));
>
> Why not put it into one line?

No reason. Will fix.

>
>> +
>> +	t2_curr_l = rd32(IGC_PTM_CURR_T2_L);
>> +	t2_curr_h = rd32(IGC_PTM_CURR_T2_H);
>> +
>> +	/* FIXME: When the register that tells the endianness of the
>> +	 * PTM registers are implemented, check them here and add the
>> +	 * appropriate conversion.
>> +	 */
>> +	t2_curr_h = swab32(t2_curr_h);
>> +
>> +	t2_curr = ((s64)t2_curr_h << 32 | t2_curr_l);
>> +
>> +	t4mt1_prev = rd32(IGC_PTM_PREV_T4M1);
>> +	t3mt2_prev = rd32(IGC_PTM_PREV_T3M2);
>> +
>> +	delay = (t4mt1_prev - t3mt2_prev) / 2;
>> +
>> +	*device = t1 + delay;
>> +	*system = igc_device_tstamp_to_system(t2_curr);
>> +
>> +	return 0;
>> +}
>> +
>> +static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
>> +				  struct system_device_crosststamp *cts)
>> +{
>> +	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
>> +						   ptp_caps);
>> +
>> +	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
>> +					     adapter, &adapter->snapshot, cts);
>> +}
>> +
>>   /**
>>    * igc_ptp_init - Initialize PTP functionality
>>    * @adapter: Board private structure
>> @@ -788,6 +937,11 @@ void igc_ptp_init(struct igc_adapter *adapter)
>>   		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
>>   		adapter->ptp_caps.n_pins = IGC_N_SDP;
>>   		adapter->ptp_caps.verify = igc_ptp_verify_pin;
>> +
>> +		if (!igc_is_crosststamp_supported(adapter))
>> +			break;
>> +
>> +		adapter->ptp_caps.getcrosststamp = igc_ptp_getcrosststamp;
>>   		break;
>>   	default:
>>   		adapter->ptp_clock = NULL;
>> @@ -878,7 +1032,9 @@ void igc_ptp_stop(struct igc_adapter *adapter)
>>   void igc_ptp_reset(struct igc_adapter *adapter)
>>   {
>>   	struct igc_hw *hw = &adapter->hw;
>> +	u32 cycle_ctrl, ctrl;
>>   	unsigned long flags;
>> +	u32 timadj;
>>   
>>   	/* reset the tstamp_config */
>>   	igc_ptp_set_timestamp_mode(adapter, &adapter->tstamp_config);
>> @@ -887,12 +1043,38 @@ void igc_ptp_reset(struct igc_adapter *adapter)
>>   
>>   	switch (adapter->hw.mac.type) {
>>   	case igc_i225:
>> +		timadj = rd32(IGC_TIMADJ);
>> +		timadj |= IGC_TIMADJ_ADJUST_METH;
>> +		wr32(IGC_TIMADJ, timadj);
>> +
>>   		wr32(IGC_TSAUXC, 0x0);
>>   		wr32(IGC_TSSDP, 0x0);
>>   		wr32(IGC_TSIM,
>>   		     IGC_TSICR_INTERRUPTS |
>>   		     (adapter->pps_sys_wrap_on ? IGC_TSICR_SYS_WRAP : 0));
>>   		wr32(IGC_IMS, IGC_IMS_TS);
>> +
>> +		if (!igc_is_crosststamp_supported(adapter))
>> +			break;
>> +
>> +		wr32(IGC_PCIE_DIG_DELAY, IGC_PCIE_DIG_DELAY_DEFAULT);
>> +		wr32(IGC_PCIE_PHY_DELAY, IGC_PCIE_PHY_DELAY_DEFAULT);
>> +
>> +		cycle_ctrl = IGC_PTM_CYCLE_CTRL_CYC_TIME(IGC_PTM_CYC_TIME_DEFAULT);
>> +
>> +		wr32(IGC_PTM_CYCLE_CTRL, cycle_ctrl);
>> +
>> +		ctrl = IGC_PTM_CTRL_EN |
>> +			IGC_PTM_CTRL_START_NOW |
>> +			IGC_PTM_CTRL_SHRT_CYC(IGC_PTM_SHORT_CYC_DEFAULT) |
>> +			IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT) |
>> +			IGC_PTM_CTRL_TRIG;
>> +
>> +		wr32(IGC_PTM_CTRL, ctrl);
>> +
>> +		/* Force the first cycle to run. */
>> +		wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
>> +
>>   		break;
>>   	default:
>>   		/* No work to do. */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
>> index 0f82990567d9..4499a6f7c577 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
>> @@ -229,6 +229,29 @@
>>   #define IGC_TXSTMPL	0x0B618  /* Tx timestamp value Low - RO */
>>   #define IGC_TXSTMPH	0x0B61C  /* Tx timestamp value High - RO */
>>   
>> +#define IGC_TIMADJ	0x0B60C  /* Time Adjustment Offset Register */
>> +
>> +/* PCIe Registers */
>> +#define IGC_PTM_CTRL		0x12540  /* PTM Control */
>> +#define IGC_PTM_STAT		0x12544  /* PTM Status */
>> +#define IGC_PTM_CYCLE_CTRL	0x1254C  /* PTM Cycle Control */
>> +
>> +/* PTM Time registers */
>> +#define IGC_PTM_T1_TIM0_L	0x12558  /* T1 on Timer 0 Low */
>> +#define IGC_PTM_T1_TIM0_H	0x1255C  /* T1 on Timer 0 High */
>> +
>> +#define IGC_PTM_CURR_T2_L	0x1258C  /* Current T2 Low */
>> +#define IGC_PTM_CURR_T2_H	0x12590  /* Current T2 High */
>> +#define IGC_PTM_PREV_T2_L	0x12584  /* Previous T2 Low */
>> +#define IGC_PTM_PREV_T2_H	0x12588  /* Previous T2 High */
>> +#define IGC_PTM_PREV_T4M1	0x12578  /* T4 Minus T1 on previous PTM Cycle */
>> +#define IGC_PTM_CURR_T4M1	0x1257C  /* T4 Minus T1 on this PTM Cycle */
>> +#define IGC_PTM_PREV_T3M2	0x12580  /* T3 Minus T2 on previous PTM Cycle */
>> +#define IGC_PTM_TDELAY		0x12594  /* PTM PCIe Link Delay */
>> +
>> +#define IGC_PCIE_DIG_DELAY	0x12550  /* PCIe Digital Delay */
>> +#define IGC_PCIE_PHY_DELAY	0x12554  /* PCIe PHY Delay */
>> +
>>   /* Management registers */
>>   #define IGC_MANC	0x05820  /* Management Control - RW */
>>   
>
> How can the user find out, that PTP grecroosstamp() is used?

If by user you mean the system administrator or similar, they could look
at the output of phc2sys, they would see that the "delay" (meaning the
delay between the device and system timestamps) is going to be zero.

If you mean applications in general, they could check the return value
of the PTP_SYS_OFFSET_PRECISE(2) ioctl(), it's going to return not
supported if getcrosststamp() is not used.

>
>
> Kind regards,
>
> Paul


Cheers,
-- 
Vinicius