[PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Prasad Malisetty]

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

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

Re: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Stephen Boyd]

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;

RE: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Prasad Malisetty (Temp)]

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;

RE: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Stephen Boyd]

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?

Re: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Bjorn Helgaas]

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
> 

Re: [PATCH v2] [RFC PATCH] PCI: Update LTR threshold based on LTRME bit

[Bjorn Helgaas]

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

[PATCH v3] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna chaitanya chundru]

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.

Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
reported snoop/no-snoop values is greather than or equal to
LTR_L1.2_THRESHOLD value.

Suggested-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---

I am takking this patch forward as prasad is no more working with our org.

Changes since v2:
	- Replaced LTRME logic with max snoop/no-snoop latencies check.
Changes since v1:
	- Added missing variable declaration in v1 patch
---
 drivers/pci/pcie/aspm.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a96b742..4a15e50 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -465,10 +465,19 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	u32 ctl1 = 0, ctl2 = 0;
 	u32 pctl1, pctl2, cctl1, cctl2;
 	u32 pl1_2_enables, cl1_2_enables;
+	int ltr;
+	u16 max_snoop_lat = 0, max_nosnoop_lat = 0;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
 
+	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return;
+
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
+
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
@@ -501,7 +510,18 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	 */
 	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;
+
+	/*
+	 * If the max snoop and no snoop latencies are '0', then avoid updating scale
+	 * and value.
+	 *
+	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
+	 * snoop/no-snoop values is greather than or equal to LTR_L1.2_THRESHOLD value.
+	 */
+	if ((max_snoop_lat == 0) && (max_nosnoop_lat == 0))
+		ctl1 |= t_common_mode << 8;
+	else
+		ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 
 	/* Some broken devices only support dword access to L1 SS */
 	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
-- 
2.7.4

Re: [PATCH v3] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna Chaitanya Chundru]

[+cc kenny, vidya]

On 6/1/2022 5:53 PM, Krishna chaitanya chundru wrote:
> 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.
>
> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> reported snoop/no-snoop values is greather than or equal to
> LTR_L1.2_THRESHOLD value.
>
> Suggested-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>
> I am takking this patch forward as prasad is no more working with our org.
>
> Changes since v2:
> 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
> Changes since v1:
> 	- Added missing variable declaration in v1 patch
> ---
>   drivers/pci/pcie/aspm.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b742..4a15e50 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -465,10 +465,19 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   	u32 ctl1 = 0, ctl2 = 0;
>   	u32 pctl1, pctl2, cctl1, cctl2;
>   	u32 pl1_2_enables, cl1_2_enables;
> +	int ltr;
> +	u16 max_snoop_lat = 0, max_nosnoop_lat = 0;
>   
>   	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>   		return;
>   
> +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> +	if (!ltr)
> +		return;
> +
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> +
>   	/* Choose the greater of the two Port Common_Mode_Restore_Times */
>   	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>   	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> @@ -501,7 +510,18 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   	 */
>   	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;
> +
> +	/*
> +	 * If the max snoop and no snoop latencies are '0', then avoid updating scale
> +	 * and value.
> +	 *
> +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> +	 * snoop/no-snoop values is greather than or equal to LTR_L1.2_THRESHOLD value.
> +	 */
> +	if ((max_snoop_lat == 0) && (max_nosnoop_lat == 0))
> +		ctl1 |= t_common_mode << 8;
> +	else
> +		ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>   
>   	/* Some broken devices only support dword access to L1 SS */
>   	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);

Re: [PATCH v3] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Manivannan Sadhasivam]

On Wed, Jun 01, 2022 at 05:57:53PM +0530, Krishna Chaitanya Chundru wrote:
> [+cc kenny, vidya]
> 
> On 6/1/2022 5:53 PM, Krishna chaitanya chundru wrote:
> > In ASPM driver, LTR threshold scale and value is updating based on

s/is/are

s/updating/updated

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

s/is/are

> > value.
> > 
> > Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> > reported snoop/no-snoop values is greather than or equal to
> > LTR_L1.2_THRESHOLD value.
> > 
> > Suggested-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

If you are inheriting the patch from Prasad, then you should still give the
authorship to him (unless the patch has changed significantly). You can add
your S-o-b tag to convey that you are carrying the patch from him.

> > ---
> > 
> > I am takking this patch forward as prasad is no more working with our org.
> > 
> > Changes since v2:
> > 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
> > Changes since v1:
> > 	- Added missing variable declaration in v1 patch
> > ---
> >   drivers/pci/pcie/aspm.c | 22 +++++++++++++++++++++-
> >   1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a96b742..4a15e50 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -465,10 +465,19 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> >   	u32 ctl1 = 0, ctl2 = 0;
> >   	u32 pctl1, pctl2, cctl1, cctl2;
> >   	u32 pl1_2_enables, cl1_2_enables;
> > +	int ltr;

This could be u16 too.

> > +	u16 max_snoop_lat = 0, max_nosnoop_lat = 0;

No need to initialize these variables.

> >   	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
> >   		return;
> > +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> > +	if (!ltr)
> > +		return;

Is this capability implemented always?

> > +
> > +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> > +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> > +
> >   	/* Choose the greater of the two Port Common_Mode_Restore_Times */
> >   	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> >   	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> > @@ -501,7 +510,18 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> >   	 */
> >   	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;
> > +
> > +	/*
> > +	 * If the max snoop and no snoop latencies are '0', then avoid updating scale
> > +	 * and value.
> > +	 *

This looks fine but...

> > +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> > +	 * snoop/no-snoop values is greather than or equal to LTR_L1.2_THRESHOLD value.

s/is/are

What about this? What if the snoop/nosnoop latencies are not equal to zero and
lower than LTR_L1.2_THRESHOLD?

Thanks,
Mani

> > +	 */
> > +	if ((max_snoop_lat == 0) && (max_nosnoop_lat == 0))
> > +		ctl1 |= t_common_mode << 8;
> > +	else
> > +		ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> >   	/* Some broken devices only support dword access to L1 SS */
> >   	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna Chaitanya Chundru]

On 6/2/2022 1:59 PM, Manivannan Sadhasivam wrote:
> On Wed, Jun 01, 2022 at 05:57:53PM +0530, Krishna Chaitanya Chundru wrote:
>> [+cc kenny, vidya]
>>
>> On 6/1/2022 5:53 PM, Krishna chaitanya chundru wrote:
>>> In ASPM driver, LTR threshold scale and value is updating based on
> s/is/are
>
> s/updating/updated
>
>>> 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
> s/is/are
>
>>> value.
>>>
>>> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
>>> reported snoop/no-snoop values is greather than or equal to
>>> LTR_L1.2_THRESHOLD value.
>>>
>>> Suggested-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> If you are inheriting the patch from Prasad, then you should still give the
> authorship to him (unless the patch has changed significantly). You can add
> your S-o-b tag to convey that you are carrying the patch from him.
Thanks mani for pointing this, I will modify this in next patch.
>>> ---
>>>
>>> I am takking this patch forward as prasad is no more working with our org.
>>>
>>> Changes since v2:
>>> 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
>>> Changes since v1:
>>> 	- Added missing variable declaration in v1 patch
>>> ---
>>>    drivers/pci/pcie/aspm.c | 22 +++++++++++++++++++++-
>>>    1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index a96b742..4a15e50 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -465,10 +465,19 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>    	u32 ctl1 = 0, ctl2 = 0;
>>>    	u32 pctl1, pctl2, cctl1, cctl2;
>>>    	u32 pl1_2_enables, cl1_2_enables;
>>> +	int ltr;
> This could be u16 too.
Will change in the next patch
>>> +	u16 max_snoop_lat = 0, max_nosnoop_lat = 0;
> No need to initialize these variables.
I will update these in next patch.
>>>    	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>>>    		return;
>>> +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
>>> +	if (!ltr)
>>> +		return;
> Is this capability implemented always?

Based up on spec 4.1, sec 5.5 Ports that support the L1.2 substate for 
ASPM L1 must support this.

And there is already a check in this function  if there is no L1.2 
support the function is returning.

>>> +
>>> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
>>> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
>>> +
>>>    	/* Choose the greater of the two Port Common_Mode_Restore_Times */
>>>    	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>>>    	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>>> @@ -501,7 +510,18 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>    	 */
>>>    	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;
>>> +
>>> +	/*
>>> +	 * If the max snoop and no snoop latencies are '0', then avoid updating scale
>>> +	 * and value.
>>> +	 *
> This looks fine but...
>
>>> +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
>>> +	 * snoop/no-snoop values is greather than or equal to LTR_L1.2_THRESHOLD value.
> s/is/are
>
> What about this? What if the snoop/nosnoop latencies are not equal to zero and
> lower than LTR_L1.2_THRESHOLD?
>
> Thanks,
> Mani

Will address this in next patch.

Thanks,

Krishna Chaitanya.

>>> +	 */
>>> +	if ((max_snoop_lat == 0) && (max_nosnoop_lat == 0))
>>> +		ctl1 |= t_common_mode << 8;
>>> +	else
>>> +		ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>>>    	/* Some broken devices only support dword access to L1 SS */
>>>    	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);

[PATCH v4] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna chaitanya chundru]

From: Prasad Malisetty <quic_pmaliset@quicinc.com>

In ASPM driver, LTR threshold scale and value are updated based on
tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
LTR threshold scale and value are greater values than max snoop/non-snoop
value.

Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
reported snoop/no-snoop values is greather than or equal to
LTR_L1.2_THRESHOLD value.

Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---

I am taking this patch forward as prasad is no more working with our org.
changes since v3:
	- Changed the logic to include this condition "snoop/nosnoop
	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
Changes since v2:
	- Replaced LTRME logic with max snoop/no-snoop latencies check.
Changes since v1:
	- Added missing variable declaration in v1 patch
---
 drivers/pci/pcie/aspm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a96b742..c8f6253 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
+	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 	u32 ctl1 = 0, ctl2 = 0;
 	u32 pctl1, pctl2, cctl1, cctl2;
 	u32 pl1_2_enables, cl1_2_enables;
+	u16 ltr;
+	u16 max_snoop_lat, max_nosnoop_lat;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
 
+	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return;
+
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
+
+	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
+	max_snp_val = (max_snoop_lat & PCI_LTR_VALUE_MASK);
+
+	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
+	max_nsnp_val = (max_nosnoop_lat & PCI_LTR_VALUE_MASK);
+
+	/* choose the greater max scale value between snoop and no snoop value*/
+	max_scale = (max_snp_scale > max_nsnp_scale) ? max_snp_scale: max_nsnp_scale;
+
+	/* choose the greater max value between snoop and no snoop scales */
+	max_val = (max_snp_val > max_nsnp_val) ? max_snp_val: max_nsnp_val;
+
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
@@ -501,6 +523,16 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
+
+	/*
+	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
+	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
+	 */
+	if (scale > max_scale)
+		scale = max_scale;
+	if (value > max_val)
+		value = max_val;
+
 	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 
 	/* Some broken devices only support dword access to L1 SS */
-- 
2.7.4

Re: [PATCH v4] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Stephen Boyd]

Quoting Krishna chaitanya chundru (2022-06-03 00:54:19)
> From: Prasad Malisetty <quic_pmaliset@quicinc.com>
>
> In ASPM driver, LTR threshold scale and value are updated based on
> tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
> LTR threshold scale and value are greater values than max snoop/non-snoop
> value.
>
> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> reported snoop/no-snoop values is greather than or equal to
> LTR_L1.2_THRESHOLD value.
>
> Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>
> I am taking this patch forward as prasad is no more working with our org.

Not sure why it's a reply to the previous rounds. I didn't notice this
patch for a bit. Can you stop sending as replies to the previous round?

> changes since v3:
>         - Changed the logic to include this condition "snoop/nosnoop
>           latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
> Changes since v2:
>         - Replaced LTRME logic with max snoop/no-snoop latencies check.
> Changes since v1:
>         - Added missing variable declaration in v1 patch
> ---
>  drivers/pci/pcie/aspm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b742..c8f6253 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  {
>         struct pci_dev *child = link->downstream, *parent = link->pdev;
>         u32 val1, val2, scale1, scale2;
> +       u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
>         u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
>         u32 ctl1 = 0, ctl2 = 0;
>         u32 pctl1, pctl2, cctl1, cctl2;
>         u32 pl1_2_enables, cl1_2_enables;
> +       u16 ltr;
> +       u16 max_snoop_lat, max_nosnoop_lat;
>
>         if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>                 return;
>
> +       ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> +       if (!ltr)
> +               return;
> +
> +       pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> +       pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> +
> +       max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +       max_snp_val = (max_snoop_lat & PCI_LTR_VALUE_MASK);

Remove useless parenthesis please.

> +
> +       max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +       max_nsnp_val = (max_nosnoop_lat & PCI_LTR_VALUE_MASK)

Remove useless parenthesis please.

> +
> +       /* choose the greater max scale value between snoop and no snoop value*/
> +       max_scale = (max_snp_scale > max_nsnp_scale) ? max_snp_scale: max_nsnp_scale;

Use max()?

> +
> +       /* choose the greater max value between snoop and no snoop scales */
> +       max_val = (max_snp_val > max_nsnp_val) ? max_snp_val: max_nsnp_val;

Use max()?

> +
>         /* Choose the greater of the two Port Common_Mode_Restore_Times */
>         val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>         val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> @@ -501,6 +523,16 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>          */
>         l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>         encode_l12_threshold(l1_2_threshold, &scale, &value);
> +
> +       /*
> +        * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> +        * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
> +        */
> +       if (scale > max_scale)
> +               scale = max_scale;

Use min()?

> +       if (value > max_val)
> +               value = max_val;

Use min()?

> +
>         ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>
>         /* Some broken devices only support dword access to L1 SS */

[PATCH v5] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna chaitanya chundru]

From: Prasad Malisetty <quic_pmaliset@quicinc.com>

In ASPM driver, LTR threshold scale and value are updated based on
tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
LTR threshold scale and value are greater values than max snoop/non-snoop
value.

Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
reported snoop/no-snoop values is greather than or equal to
LTR_L1.2_THRESHOLD value.

Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---

I am taking this patch forward as prasad is no more working with our org.
Changes since v4:
	- Replaced conditional statements with min and max.
changes since v3:
	- Changed the logic to include this condition "snoop/nosnoop
	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
Changes since v2:
	- Replaced LTRME logic with max snoop/no-snoop latencies check.
Changes since v1:
	- Added missing variable declaration in v1 patch
---
 drivers/pci/pcie/aspm.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a96b742..676c03e 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
+	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 	u32 ctl1 = 0, ctl2 = 0;
 	u32 pctl1, pctl2, cctl1, cctl2;
 	u32 pl1_2_enables, cl1_2_enables;
+	u16 ltr;
+	u16 max_snoop_lat, max_nosnoop_lat;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
 
+	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return;
+
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
+	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
+
+	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
+	max_snp_val = max_snoop_lat & PCI_LTR_VALUE_MASK;
+
+	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
+	max_nsnp_val = max_nosnoop_lat & PCI_LTR_VALUE_MASK;
+
+	/* choose the greater max scale value between snoop and no snoop value*/
+	max_scale = max(max_snp_scale, max_nsnp_scale);
+
+	/* choose the greater max value between snoop and no snoop scales */
+	max_val = max(max_snp_val, max_nsnp_val);
+
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
@@ -501,6 +523,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
+
+	/*
+	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
+	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
+	 */
+	scale = min(scale, max_scale);
+	value = min(value, max_val);
+
 	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 
 	/* Some broken devices only support dword access to L1 SS */
-- 
2.7.4

Re: [PATCH v5] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna Chaitanya Chundru]

A Gentle remainder please review it.

On 6/10/2022 10:38 AM, Krishna chaitanya chundru wrote:
> From: Prasad Malisetty <quic_pmaliset@quicinc.com>
>
> In ASPM driver, LTR threshold scale and value are updated based on
> tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
> LTR threshold scale and value are greater values than max snoop/non-snoop
> value.
>
> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> reported snoop/no-snoop values is greather than or equal to
> LTR_L1.2_THRESHOLD value.
>
> Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>
> I am taking this patch forward as prasad is no more working with our org.
> Changes since v4:
> 	- Replaced conditional statements with min and max.
> changes since v3:
> 	- Changed the logic to include this condition "snoop/nosnoop
> 	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
> Changes since v2:
> 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
> Changes since v1:
> 	- Added missing variable declaration in v1 patch
> ---
>   drivers/pci/pcie/aspm.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b742..676c03e 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   {
>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
>   	u32 val1, val2, scale1, scale2;
> +	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
>   	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
>   	u32 ctl1 = 0, ctl2 = 0;
>   	u32 pctl1, pctl2, cctl1, cctl2;
>   	u32 pl1_2_enables, cl1_2_enables;
> +	u16 ltr;
> +	u16 max_snoop_lat, max_nosnoop_lat;
>   
>   	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>   		return;
>   
> +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> +	if (!ltr)
> +		return;
> +
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> +
> +	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +	max_snp_val = max_snoop_lat & PCI_LTR_VALUE_MASK;
> +
> +	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +	max_nsnp_val = max_nosnoop_lat & PCI_LTR_VALUE_MASK;
> +
> +	/* choose the greater max scale value between snoop and no snoop value*/
> +	max_scale = max(max_snp_scale, max_nsnp_scale);
> +
> +	/* choose the greater max value between snoop and no snoop scales */
> +	max_val = max(max_snp_val, max_nsnp_val);
> +
>   	/* Choose the greater of the two Port Common_Mode_Restore_Times */
>   	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>   	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> @@ -501,6 +523,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   	 */
>   	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>   	encode_l12_threshold(l1_2_threshold, &scale, &value);
> +
> +	/*
> +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> +	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
> +	 */
> +	scale = min(scale, max_scale);
> +	value = min(value, max_val);
> +
>   	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>   
>   	/* Some broken devices only support dword access to L1 SS */

Re: [PATCH v5] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Manivannan Sadhasivam]

On Wed, Jun 15, 2022 at 06:53:18PM +0530, Krishna Chaitanya Chundru wrote:
> A Gentle remainder please review it.
> 
> On 6/10/2022 10:38 AM, Krishna chaitanya chundru wrote:
> > From: Prasad Malisetty <quic_pmaliset@quicinc.com>
> > 
> > In ASPM driver, LTR threshold scale and value are updated based on
> > tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
> > LTR threshold scale and value are greater values than max snoop/non-snoop
> > value.
> > 
> > Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> > reported snoop/no-snoop values is greather than or equal to
> > LTR_L1.2_THRESHOLD value.
> > 
> > Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

As Stephen noted in previous verison, you should sent the newer versions
separately and not as a reply to previous ones. So please sent v6 with
my tag as well.

Thanks,
Mani

> > ---
> > 
> > I am taking this patch forward as prasad is no more working with our org.
> > Changes since v4:
> > 	- Replaced conditional statements with min and max.
> > changes since v3:
> > 	- Changed the logic to include this condition "snoop/nosnoop
> > 	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
> > Changes since v2:
> > 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
> > Changes since v1:
> > 	- Added missing variable declaration in v1 patch
> > ---
> >   drivers/pci/pcie/aspm.c | 30 ++++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a96b742..676c03e 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> >   {
> >   	struct pci_dev *child = link->downstream, *parent = link->pdev;
> >   	u32 val1, val2, scale1, scale2;
> > +	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
> >   	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
> >   	u32 ctl1 = 0, ctl2 = 0;
> >   	u32 pctl1, pctl2, cctl1, cctl2;
> >   	u32 pl1_2_enables, cl1_2_enables;
> > +	u16 ltr;
> > +	u16 max_snoop_lat, max_nosnoop_lat;
> >   	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
> >   		return;
> > +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> > +	if (!ltr)
> > +		return;
> > +
> > +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> > +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> > +
> > +	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> > +	max_snp_val = max_snoop_lat & PCI_LTR_VALUE_MASK;
> > +
> > +	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> > +	max_nsnp_val = max_nosnoop_lat & PCI_LTR_VALUE_MASK;
> > +
> > +	/* choose the greater max scale value between snoop and no snoop value*/
> > +	max_scale = max(max_snp_scale, max_nsnp_scale);
> > +
> > +	/* choose the greater max value between snoop and no snoop scales */
> > +	max_val = max(max_snp_val, max_nsnp_val);
> > +
> >   	/* Choose the greater of the two Port Common_Mode_Restore_Times */
> >   	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> >   	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> > @@ -501,6 +523,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> >   	 */
> >   	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
> >   	encode_l12_threshold(l1_2_threshold, &scale, &value);
> > +
> > +	/*
> > +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> > +	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
> > +	 */
> > +	scale = min(scale, max_scale);
> > +	value = min(value, max_val);
> > +
> >   	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> >   	/* Some broken devices only support dword access to L1 SS */

Re: [PATCH v5] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Krishna Chaitanya Chundru]

On 7/15/2022 1:58 PM, Manivannan Sadhasivam wrote:
> On Wed, Jun 15, 2022 at 06:53:18PM +0530, Krishna Chaitanya Chundru wrote:
>> A Gentle remainder please review it.
>>
>> On 6/10/2022 10:38 AM, Krishna chaitanya chundru wrote:
>>> From: Prasad Malisetty <quic_pmaliset@quicinc.com>
>>>
>>> In ASPM driver, LTR threshold scale and value are updated based on
>>> tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
>>> LTR threshold scale and value are greater values than max snoop/non-snoop
>>> value.
>>>
>>> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
>>> reported snoop/no-snoop values is greather than or equal to
>>> LTR_L1.2_THRESHOLD value.
>>>
>>> Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> As Stephen noted in previous verison, you should sent the newer versions
> separately and not as a reply to previous ones. So please sent v6 with
> my tag as well.
>
> Thanks,
> Mani

ok mani I will send the v6 as you suggested.


Thanks

Krishna chaitanya.

>
>>> ---
>>>
>>> I am taking this patch forward as prasad is no more working with our org.
>>> Changes since v4:
>>> 	- Replaced conditional statements with min and max.
>>> changes since v3:
>>> 	- Changed the logic to include this condition "snoop/nosnoop
>>> 	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
>>> Changes since v2:
>>> 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
>>> Changes since v1:
>>> 	- Added missing variable declaration in v1 patch
>>> ---
>>>    drivers/pci/pcie/aspm.c | 30 ++++++++++++++++++++++++++++++
>>>    1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index a96b742..676c03e 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>    {
>>>    	struct pci_dev *child = link->downstream, *parent = link->pdev;
>>>    	u32 val1, val2, scale1, scale2;
>>> +	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
>>>    	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
>>>    	u32 ctl1 = 0, ctl2 = 0;
>>>    	u32 pctl1, pctl2, cctl1, cctl2;
>>>    	u32 pl1_2_enables, cl1_2_enables;
>>> +	u16 ltr;
>>> +	u16 max_snoop_lat, max_nosnoop_lat;
>>>    	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>>>    		return;
>>> +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
>>> +	if (!ltr)
>>> +		return;
>>> +
>>> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
>>> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
>>> +
>>> +	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
>>> +	max_snp_val = max_snoop_lat & PCI_LTR_VALUE_MASK;
>>> +
>>> +	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
>>> +	max_nsnp_val = max_nosnoop_lat & PCI_LTR_VALUE_MASK;
>>> +
>>> +	/* choose the greater max scale value between snoop and no snoop value*/
>>> +	max_scale = max(max_snp_scale, max_nsnp_scale);
>>> +
>>> +	/* choose the greater max value between snoop and no snoop scales */
>>> +	max_val = max(max_snp_val, max_nsnp_val);
>>> +
>>>    	/* Choose the greater of the two Port Common_Mode_Restore_Times */
>>>    	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>>>    	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>>> @@ -501,6 +523,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>    	 */
>>>    	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>>>    	encode_l12_threshold(l1_2_threshold, &scale, &value);
>>> +
>>> +	/*
>>> +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
>>> +	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
>>> +	 */
>>> +	scale = min(scale, max_scale);
>>> +	value = min(value, max_val);
>>> +
>>>    	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>>>    	/* Some broken devices only support dword access to L1 SS */

Re: [PATCH v5] PCI/ASPM: Update LTR threshold based upon reported max latencies

[Manivannan Sadhasivam]

On Fri, Jun 10, 2022 at 10:38:28AM +0530, Krishna chaitanya chundru wrote:
> From: Prasad Malisetty <quic_pmaliset@quicinc.com>
> 
> In ASPM driver, LTR threshold scale and value are updated based on
> tcommon_mode and t_poweron values. In kioxia NVMe L1.2 is failing due to
> LTR threshold scale and value are greater values than max snoop/non-snoop
> value.
> 
> Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when
> reported snoop/no-snoop values is greather than or equal to
> LTR_L1.2_THRESHOLD value.
> 
> Signed-off-by: Prasad Malisetty  <quic_pmaliset@quicinc.com>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
> 
> I am taking this patch forward as prasad is no more working with our org.
> Changes since v4:
> 	- Replaced conditional statements with min and max.
> changes since v3:
> 	- Changed the logic to include this condition "snoop/nosnoop
> 	  latencies are not equal to zero and lower than LTR_L1.2_THRESHOLD"
> Changes since v2:
> 	- Replaced LTRME logic with max snoop/no-snoop latencies check.
> Changes since v1:
> 	- Added missing variable declaration in v1 patch
> ---
>  drivers/pci/pcie/aspm.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b742..676c03e 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -461,14 +461,36 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  {
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>  	u32 val1, val2, scale1, scale2;
> +	u32 max_val, max_scale, max_snp_scale, max_snp_val, max_nsnp_scale, max_nsnp_val;
>  	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
>  	u32 ctl1 = 0, ctl2 = 0;
>  	u32 pctl1, pctl2, cctl1, cctl2;
>  	u32 pl1_2_enables, cl1_2_enables;
> +	u16 ltr;
> +	u16 max_snoop_lat, max_nosnoop_lat;
>  
>  	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>  		return;
>  
> +	ltr = pci_find_ext_capability(child, PCI_EXT_CAP_ID_LTR);
> +	if (!ltr)
> +		return;
> +
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_SNOOP_LAT, &max_snoop_lat);
> +	pci_read_config_word(child, ltr + PCI_LTR_MAX_NOSNOOP_LAT, &max_nosnoop_lat);
> +
> +	max_snp_scale = (max_snoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +	max_snp_val = max_snoop_lat & PCI_LTR_VALUE_MASK;
> +
> +	max_nsnp_scale = (max_nosnoop_lat & PCI_LTR_SCALE_MASK) >> PCI_LTR_SCALE_SHIFT;
> +	max_nsnp_val = max_nosnoop_lat & PCI_LTR_VALUE_MASK;
> +
> +	/* choose the greater max scale value between snoop and no snoop value*/
> +	max_scale = max(max_snp_scale, max_nsnp_scale);
> +
> +	/* choose the greater max value between snoop and no snoop scales */
> +	max_val = max(max_snp_val, max_nsnp_val);
> +
>  	/* Choose the greater of the two Port Common_Mode_Restore_Times */
>  	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>  	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> @@ -501,6 +523,14 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	 */
>  	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>  	encode_l12_threshold(l1_2_threshold, &scale, &value);
> +
> +	/*
> +	 * Based on PCIe r4.1, sec 5.5.1, L1.2 substate must be entered when reported
> +	 * snoop/no-snoop values are greather than or equal to LTR_L1.2_THRESHOLD value.
> +	 */
> +	scale = min(scale, max_scale);
> +	value = min(value, max_val);
> +
>  	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>  
>  	/* Some broken devices only support dword access to L1 SS */
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்