| Message ID | 20230717171927.78516-1-sasha.neftin@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v1,1/1] igc: Correct the short interval between PTM requests. | expand |
[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
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
[+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 */
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 */
[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
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 */
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(-)