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

From: "Neftin, Sasha" <sasha.neftin@intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Paul Menzel <pmenzel@molgen.mpg.de>,
	linux-pci@vger.kernel.org,
	"Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Correct the short interval between PTM requests.
Date: Tue, 18 Jul 2023 20:10:38 +0300	[thread overview]
Message-ID: <06c17403-fb5e-ff14-4707-829ede312336@intel.com> (raw)
In-Reply-To: <20230718164544.GA486141@bhelgaas>

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