| Message ID | 1646679549-12494-1-git-send-email-quic_pmaliset@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2,RFC] PCI: Update LTR threshold based on LTRME bit | expand |
Quoting Prasad Malisetty (2022-03-07 10:59:09) > Update LTR threshold scale and value based on LTRME (Latency > Tolenrance Reporting Mechanism) from device capabilities. > > In ASPM driver, LTR threshold scale and value is updating > based on tcommon_mode and t_poweron values. In kioxia NVMe, > L1.2 is failing due to LTR threshold scale and value is > greater values than max snoop/non snoop value. > > In general, updated LTR threshold scale and value should be > less than max snoop/non snoop value to enter the device > into L1.2 state. > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > Any Fixes tag? > --- > Changes since v1: > - Added missing variable declaration in v1 patch. > --- > drivers/pci/pcie/aspm.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index a96b742..a67746c 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -463,6 +463,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > u32 val1, val2, scale1, scale2; > u32 t_common_mode, t_power_on, l1_2_threshold, scale, value; > u32 ctl1 = 0, ctl2 = 0; > + u32 cap; > u32 pctl1, pctl2, cctl1, cctl2; > u32 pl1_2_enables, cl1_2_enables; > > @@ -499,9 +500,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at > * least 4us. Can this comment be updated to include why LTR cap matters? > */ > - l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > - encode_l12_threshold(l1_2_threshold, &scale, &value); > - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > + pcie_capability_read_dword(child, PCI_EXP_DEVCAP2, &cap); > + if (!(cap & PCI_EXP_DEVCAP2_LTR)) { > + l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > + encode_l12_threshold(l1_2_threshold, &scale, &value); > + ctl1 |= scale << 29 | value << 16; > + } > + > + ctl1 |= t_common_mode;
Hi Stephen, Thanks for the review and comments. Please find my comments inline below. Thanks -Prasad > -----Original Message----- > From: Stephen Boyd <swboyd@chromium.org> > Sent: Friday, March 18, 2022 12:37 AM > To: Prasad Malisetty (Temp) (QUIC) <quic_pmaliset@quicinc.com>; > agross@kernel.org; bhelgaas@google.com; bjorn.andersson@linaro.org; > kw@linux.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-pci@vger.kernel.org; lorenzo.pieralisi@arm.com; rajatja@google.com; > refactormyself@gmail.com; robh@kernel.org > Cc: Veerabhadrarao Badiganti (QUIC) <quic_vbadigan@quicinc.com>; Rama > Krishna (QUIC) <quic_ramkri@quicinc.com>; > manivannan.sadhasivam@linaro.org > Subject: Re: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on > LTRME bit > > WARNING: This email originated from outside of Qualcomm. Please be wary of > any links or attachments, and do not enable macros. > > Quoting Prasad Malisetty (2022-03-07 10:59:09) > > Update LTR threshold scale and value based on LTRME (Latency > > Tolenrance Reporting Mechanism) from device capabilities. > > > > In ASPM driver, LTR threshold scale and value is updating based on > > tcommon_mode and t_poweron values. In kioxia NVMe, > > L1.2 is failing due to LTR threshold scale and value is greater values > > than max snoop/non snoop value. > > > > In general, updated LTR threshold scale and value should be less than > > max snoop/non snoop value to enter the device into L1.2 state. > > > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > > > > Any Fixes tag? No, we don’t have any fixes tag as this is new issue identified in kioxia NVMe only as of now. > > > --- > > Changes since v1: > > - Added missing variable declaration in v1 patch. > > --- > > drivers/pci/pcie/aspm.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index > > a96b742..a67746c 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -463,6 +463,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state > *link, > > u32 val1, val2, scale1, scale2; > > u32 t_common_mode, t_power_on, l1_2_threshold, scale, value; > > u32 ctl1 = 0, ctl2 = 0; > > + u32 cap; > > u32 pctl1, pctl2, cctl1, cctl2; > > u32 pl1_2_enables, cl1_2_enables; > > > > @@ -499,9 +500,14 @@ static void aspm_calc_l1ss_info(struct > pcie_link_state *link, > > * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at > > * least 4us. > > Can this comment be updated to include why LTR cap matters? Sure, I will update the comment in next patch version. > > > */ > > - l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > > - encode_l12_threshold(l1_2_threshold, &scale, &value); > > - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > > + pcie_capability_read_dword(child, PCI_EXP_DEVCAP2, &cap); > > + if (!(cap & PCI_EXP_DEVCAP2_LTR)) { > > + l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > > + encode_l12_threshold(l1_2_threshold, &scale, &value); > > + ctl1 |= scale << 29 | value << 16; > > + } > > + > > + ctl1 |= t_common_mode;
On Tue, Mar 08, 2022 at 12:29:09AM +0530, Prasad Malisetty wrote: > Update LTR threshold scale and value based on LTRME (Latency > Tolenrance Reporting Mechanism) from device capabilities. s/Tolenrance/Tolerance/ I'm not familiar with "LTRME" and it doesn't appear in the PCI spec. Please use the PCIe terminology whenever possible. > In ASPM driver, LTR threshold scale and value is updating > based on tcommon_mode and t_poweron values. In kioxia NVMe, > L1.2 is failing due to LTR threshold scale and value is > greater values than max snoop/non snoop value. > > In general, updated LTR threshold scale and value should be > less than max snoop/non snoop value to enter the device > into L1.2 state. This affects all PCIe devices, so please include the PCIe spec citation that leads to this change. Please wrap the commit log to fill 75 columns. Thanks a lot for looking at this! L1.2 and LTR is real problem area, and it would be great if we could get it all sorted out. > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > > --- > Changes since v1: > - Added missing variable declaration in v1 patch. > --- > drivers/pci/pcie/aspm.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index a96b742..a67746c 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -463,6 +463,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > u32 val1, val2, scale1, scale2; > u32 t_common_mode, t_power_on, l1_2_threshold, scale, value; > u32 ctl1 = 0, ctl2 = 0; > + u32 cap; > u32 pctl1, pctl2, cctl1, cctl2; > u32 pl1_2_enables, cl1_2_enables; > > @@ -499,9 +500,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at > * least 4us. > */ > - l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > - encode_l12_threshold(l1_2_threshold, &scale, &value); > - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > + pcie_capability_read_dword(child, PCI_EXP_DEVCAP2, &cap); > + if (!(cap & PCI_EXP_DEVCAP2_LTR)) { > + l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > + encode_l12_threshold(l1_2_threshold, &scale, &value); > + ctl1 |= scale << 29 | value << 16; > + } > + > + ctl1 |= t_common_mode; > > /* Some broken devices only support dword access to L1 SS */ > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Quoting Prasad Malisetty (Temp) (2022-04-04 23:24:39) > > From: Stephen Boyd <swboyd@chromium.org> > > Quoting Prasad Malisetty (2022-03-07 10:59:09) > > > Update LTR threshold scale and value based on LTRME (Latency > > > Tolenrance Reporting Mechanism) from device capabilities. > > > > > > In ASPM driver, LTR threshold scale and value is updating based on > > > tcommon_mode and t_poweron values. In kioxia NVMe, > > > L1.2 is failing due to LTR threshold scale and value is greater values > > > than max snoop/non snoop value. > > > > > > In general, updated LTR threshold scale and value should be less than > > > max snoop/non snoop value to enter the device into L1.2 state. > > > > > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > > > > > > > Any Fixes tag? > No, we don’t have any fixes tag as this is new issue identified in kioxia NVMe only as of now. Just because you found it now doesn't mean it hasn't been broken for some time. Can you look through the history and figure out when it didn't work and use that commit for the fixes tag?
[+cc Vidya, Kenny] On Tue, Mar 08, 2022 at 12:29:09AM +0530, Prasad Malisetty wrote: > Update LTR threshold scale and value based on LTRME (Latency > Tolenrance Reporting Mechanism) from device capabilities. > > In ASPM driver, LTR threshold scale and value is updating > based on tcommon_mode and t_poweron values. In kioxia NVMe, > L1.2 is failing due to LTR threshold scale and value is > greater values than max snoop/non snoop value. > > In general, updated LTR threshold scale and value should be > less than max snoop/non snoop value to enter the device > into L1.2 state. Interesting that you mention an L1.2 issue on a KIOXIA NVMe device. Kenny also reported an L1 Substates issue related to a KIOXIA NVMe device at [1]. That issue happened when saving/restoring the L1 SS state for suspend/resume. We ended up reverting 4257f7e008ea to avoid the problem, but when he tested that commit later, the issue did not occur [2]. I don't know if there's a connection here, so this is just a heads-up in case there is. [1] https://lore.kernel.org/r/20201228040513.GA611645@bjorn-Precision-5520 [2] https://lore.kernel.org/r/3ca14a7-b726-8430-fe61-a3ac183a1088@panix.com > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > > --- > Changes since v1: > - Added missing variable declaration in v1 patch. > --- > drivers/pci/pcie/aspm.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index a96b742..a67746c 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -463,6 +463,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > u32 val1, val2, scale1, scale2; > u32 t_common_mode, t_power_on, l1_2_threshold, scale, value; > u32 ctl1 = 0, ctl2 = 0; > + u32 cap; > u32 pctl1, pctl2, cctl1, cctl2; > u32 pl1_2_enables, cl1_2_enables; > > @@ -499,9 +500,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at > * least 4us. > */ > - l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > - encode_l12_threshold(l1_2_threshold, &scale, &value); > - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > + pcie_capability_read_dword(child, PCI_EXP_DEVCAP2, &cap); > + if (!(cap & PCI_EXP_DEVCAP2_LTR)) { > + l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > + encode_l12_threshold(l1_2_threshold, &scale, &value); > + ctl1 |= scale << 29 | value << 16; > + } > + > + ctl1 |= t_common_mode; > > /* Some broken devices only support dword access to L1 SS */ > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index a96b742..a67746c 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -463,6 +463,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, u32 val1, val2, scale1, scale2; u32 t_common_mode, t_power_on, l1_2_threshold, scale, value; u32 ctl1 = 0, ctl2 = 0; + u32 cap; u32 pctl1, pctl2, cctl1, cctl2; u32 pl1_2_enables, cl1_2_enables; @@ -499,9 +500,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at * least 4us. */ - l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; - encode_l12_threshold(l1_2_threshold, &scale, &value); - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; + pcie_capability_read_dword(child, PCI_EXP_DEVCAP2, &cap); + if (!(cap & PCI_EXP_DEVCAP2_LTR)) { + l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; + encode_l12_threshold(l1_2_threshold, &scale, &value); + ctl1 |= scale << 29 | value << 16; + } + + ctl1 |= t_common_mode; /* Some broken devices only support dword access to L1 SS */ pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
Update LTR threshold scale and value based on LTRME (Latency Tolenrance Reporting Mechanism) from device capabilities. In ASPM driver, LTR threshold scale and value is updating based on tcommon_mode and t_poweron values. In kioxia NVMe, L1.2 is failing due to LTR threshold scale and value is greater values than max snoop/non snoop value. In general, updated LTR threshold scale and value should be less than max snoop/non snoop value to enter the device into L1.2 state. Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> --- Changes since v1: - Added missing variable declaration in v1 patch. --- drivers/pci/pcie/aspm.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)