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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Sasha Neftin <sasha.neftin@intel.com>
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 11:45:44 -0500	[thread overview]
Message-ID: <20230718164544.GA486141@bhelgaas> (raw)
In-Reply-To: <20230717171927.78516-1-sasha.neftin@intel.com>

[+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