[Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Sasha Neftin]

With the 10us interval, we were seeing PTM transactions taking around 12us.
With the 1us interval, PTM dialogs took around 2us. Checked with the PCIe
sniffer.

Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 44a507029946..c3722f524ea7 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -549,7 +549,7 @@
 #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_SHORT_CYC_DEFAULT	1   /* Default short cycle interval */
 #define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
 #define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */
 
-- 
2.34.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Paul Menzel]

[Cc: +linux-pci@vger.kernel.org]

Dear Sasha,


Thank you for your patch.

Maybe be more specific in the commit message summary. Maybe:

igc: Decrease PTM short interval from 10 us to 1 us

Am 17.07.23 um 19:19 schrieb Sasha Neftin:
> With the 10us interval, we were seeing PTM transactions taking around 12us.
> With the 1us interval, PTM dialogs took around 2us. Checked with the PCIe
> sniffer.

On what board, and with what device and what firmware versions?

> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 44a507029946..c3722f524ea7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -549,7 +549,7 @@
>   #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_SHORT_CYC_DEFAULT	1   /* Default short cycle interval */

Why is the comment updated?

>   #define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
>   #define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */


Kind regards,

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

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Neftin, Sasha]

On 7/17/2023 20:32, Paul Menzel wrote:
> [Cc: +linux-pci@vger.kernel.org]
> 
> Dear Sasha,
> 
> 
> Thank you for your patch.
> 
> Maybe be more specific in the commit message summary. Maybe:
> 
> igc: Decrease PTM short interval from 10 us to 1 us

Good.

> 
> Am 17.07.23 um 19:19 schrieb Sasha Neftin:
>> With the 10us interval, we were seeing PTM transactions taking around 
>> 12us.
>> With the 1us interval, PTM dialogs took around 2us. Checked with the PCIe
>> sniffer.
> 
> On what board, and with what device and what firmware versions?

Any with the PTM support. HW feature and not dependent on the firmware.

> 
>> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
>> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h 
>> b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 44a507029946..c3722f524ea7 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -549,7 +549,7 @@
>>   #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_SHORT_CYC_DEFAULT    1   /* Default short cycle 
>> interval */
> 
> Why is the comment updated?

Interval, not interrupt...

> 
>>   #define IGC_PTM_CYC_TIME_DEFAULT    5   /* Default PTM cycle time */
>>   #define IGC_PTM_TIMEOUT_DEFAULT        255 /* Default timeout for 
>> PTM errors */
> 
> 
> Kind regards,
> 
> Paul

Sasha
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Paul Menzel]

[Cc: +Vinicius]

Dear Sasha,


Am 18.07.23 um 18:15 schrieb Neftin, Sasha:
> On 7/17/2023 20:32, Paul Menzel wrote:
>> [Cc: +linux-pci@vger.kernel.org]

[…]

>> Maybe be more specific in the commit message summary. Maybe:
>>
>> igc: Decrease PTM short interval from 10 us to 1 us
> 
> Good.
> 
>> Am 17.07.23 um 19:19 schrieb Sasha Neftin:
>>> With the 10us interval, we were seeing PTM transactions taking around 12us.
>>> With the 1us interval, PTM dialogs took around 2us. Checked with the
>>> PCIe sniffer.
>>
>> On what board, and with what device and what firmware versions?
> 
> Any with the PTM support. HW feature and not dependent on the firmware.

Still you tested it. It’s always beneficial to at least denote one of 
the systems with all the details. Especially as it shouldn’t cost you 
more than a minute to add these details.

>>> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
>>> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h 
>>> b/drivers/net/ethernet/intel/igc/igc_defines.h
>>> index 44a507029946..c3722f524ea7 100644
>>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>>> @@ -549,7 +549,7 @@
>>>   #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_SHORT_CYC_DEFAULT    1   /* Default short cycle 
>>> interval */
>>
>> Why is the comment updated?
> 
> Interval, not interrupt...

Sorry, I do not understand your answer. It’d be great, if you elaborated.

>>>   #define IGC_PTM_CYC_TIME_DEFAULT    5   /* Default PTM cycle time */
>>>   #define IGC_PTM_TIMEOUT_DEFAULT        255 /* Default timeout for PTM errors */


Kind regards,

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

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Bjorn Helgaas]

[+cc Paul, Vinicius, Kai-Heng, Guilherme]

On Mon, Jul 17, 2023 at 08:19:27PM +0300, Sasha Neftin wrote:
> With the 10us interval, we were seeing PTM transactions taking around 12us.
> With the 1us interval, PTM dialogs took around 2us. Checked with the PCIe
> sniffer.
> 
> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 44a507029946..c3722f524ea7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -549,7 +549,7 @@
>  #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_SHORT_CYC_DEFAULT	1   /* Default short cycle interval */

Not related to *this* patch, but from looking at igc_ptp_reset(),
where IGC_PTM_SHORT_CYC_DEFAULT is used,

  /* PCIe PTM Control */
  #define IGC_PTM_CTRL_START_NOW  BIT(29) /* Start PTM Now */
  #define IGC_PTM_CTRL_EN         BIT(30) /* Enable PTM */

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

Obviously this must be implementation-specific PTM configuration,
which is fine.  But I assume even though this sets IGC_PTM_CTRL_EN and
IGC_PTM_CTRL_START_NOW, the device will not actually send PTM Request
messages unless the architected PTM Enable bit in the PTM Capability
is set (PCIe r6.0, sec 7.9.15.3).  Right?

I'm asking because Kai-Heng has been working on issues where
Unsupported Request errors are reported because some devices seem to
send PTM Requests when we don't think they should.  See
https://lore.kernel.org/r/20230714050541.2765246-1-kai.heng.feng@canonical.com

>  #define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
>  #define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.

[Neftin, Sasha]

On 7/18/2023 19:45, Bjorn Helgaas wrote:
> [+cc Paul, Vinicius, Kai-Heng, Guilherme]
> 
> On Mon, Jul 17, 2023 at 08:19:27PM +0300, Sasha Neftin wrote:
>> With the 10us interval, we were seeing PTM transactions taking around 12us.
>> With the 1us interval, PTM dialogs took around 2us. Checked with the PCIe
>> sniffer.
>>
>> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
>> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 44a507029946..c3722f524ea7 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -549,7 +549,7 @@
>>   #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_SHORT_CYC_DEFAULT	1   /* Default short cycle interval */
> 
> Not related to *this* patch, but from looking at igc_ptp_reset(),
> where IGC_PTM_SHORT_CYC_DEFAULT is used,
> 
>    /* PCIe PTM Control */
>    #define IGC_PTM_CTRL_START_NOW  BIT(29) /* Start PTM Now */
>    #define IGC_PTM_CTRL_EN         BIT(30) /* Enable PTM */
> 
>    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);
> 
> Obviously this must be implementation-specific PTM configuration,
> which is fine.  But I assume even though this sets IGC_PTM_CTRL_EN and
> IGC_PTM_CTRL_START_NOW, the device will not actually send PTM Request
> messages unless the architected PTM Enable bit in the PTM Capability
> is set (PCIe r6.0, sec 7.9.15.3).  Right?

Right.

> 
> I'm asking because Kai-Heng has been working on issues where
> Unsupported Request errors are reported because some devices seem to
> send PTM Requests when we don't think they should.  See
> https://lore.kernel.org/r/20230714050541.2765246-1-kai.heng.feng@canonical.com

I know. This is a different problem. (I can not reproduce, we will try 
to get HP dock). We will think how to help here.

> 
>>   #define IGC_PTM_CYC_TIME_DEFAULT	5   /* Default PTM cycle time */
>>   #define IGC_PTM_TIMEOUT_DEFAULT		255 /* Default timeout for PTM errors */

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan