[PATCH V4 0/2] PCI/ASPM: Save/restore L1SS Capability for suspend/resume

[PATCH V4 0/2] PCI/ASPM: Save/restore L1SS Capability for suspend/resume

[Vidya Sagar]

This patch series saves and restores the ASPM L1SS capability registers
during suspend/resume cycle of the system.
First patch refactors the existing L1SS register programming code to
take out the common code.
Second patch adds support to save and restore of the L1SS registers using
the common code extracted in the first patch to restore the registers.

This patch is verified on Tegra194 and Tegra234 platforms.

Vidya Sagar (2):
  PCI/ASPM: Refactor ASPM L1SS control register programming
  PCI/ASPM: Save/restore L1SS Capability for suspend/resume

 drivers/pci/pci.c       |   7 +++
 drivers/pci/pci.h       |   4 ++
 drivers/pci/pcie/aspm.c | 106 +++++++++++++++++++++++++++-------------
 3 files changed, 83 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH V4 1/2] PCI/ASPM: Refactor ASPM L1SS control register programming

[Vidya Sagar]

Refactor the code to extract the command code out to program
Control Registers-1 & 2 of L1 Sub-States capability to a new function
aspm_program_l1ss() and call it for both parent and child devices.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* New patch in this series

 drivers/pci/pcie/aspm.c | 63 +++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a8aec190986c..ecbe3af4188d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -455,6 +455,31 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
 	pci_write_config_dword(pdev, pos, val);
 }
 
+static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
+{
+	u16 l1ss = dev->l1ss;
+	u32 l1_2_enable;
+
+	/*
+	 * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
+	 * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
+	 */
+	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
+
+	/*
+	 * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
+	 * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
+	 * enable bits, even though they're all in PCI_L1SS_CTL1.
+	 */
+	l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+	ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
+
+	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
+	if (l1_2_enable)
+		pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
+				       ctl1 | l1_2_enable);
+}
+
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				u32 parent_l1ss_cap, u32 child_l1ss_cap)
@@ -464,7 +489,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	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;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
@@ -513,39 +537,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	    ctl2 == pctl2 && ctl2 == cctl2)
 		return;
 
-	/* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
-	pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
-	cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
-
-	if (pl1_2_enables || cl1_2_enables) {
-		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_L1_2_MASK, 0);
-		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_L1_2_MASK, 0);
-	}
-
-	/* Program T_POWER_ON times in both ports */
-	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
-	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
-
-	/* Program Common_Mode_Restore_Time in upstream device */
-	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
-
-	/* Program LTR_L1.2_THRESHOLD time in both ports */
-	pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
-				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
-	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
-				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
-
-	if (pl1_2_enables || cl1_2_enables) {
-		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
-					pl1_2_enables);
-		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
-					cl1_2_enables);
-	}
+	aspm_program_l1ss(parent,
+			  ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
+	aspm_program_l1ss(child,
+			  ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
-- 
2.17.1

[PATCH V4 2/2] PCI/ASPM: Save/restore L1SS Capability for suspend/resume

[Vidya Sagar]

Previously ASPM L1 Substates control registers (CTL1 and CTL2) weren't
saved and restored during suspend/resume leading to L1 Substates
configuration being lost post-resume.

Save the L1 Substates control registers so that the configuration is
retained post-resume.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* Address review comments from Bjorn
* Use the API aspm_program_l1ss() to restore L1SS registers

V3:
* Disabled L1.2 enable fields while restoring Control-1 register

 drivers/pci/pci.c       |  7 +++++++
 drivers/pci/pci.h       |  4 ++++
 drivers/pci/pcie/aspm.c | 43 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 95bc329e74c0..68a49fbaabde 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1663,6 +1663,7 @@ int pci_save_state(struct pci_dev *dev)
 		return i;
 
 	pci_save_ltr_state(dev);
+	pci_save_aspm_l1ss_state(dev);
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
 	pci_save_ptm_state(dev);
@@ -1769,6 +1770,7 @@ void pci_restore_state(struct pci_dev *dev)
 	 * LTR itself (in the PCIe capability).
 	 */
 	pci_restore_ltr_state(dev);
+	pci_restore_aspm_l1ss_state(dev);
 
 	pci_restore_pcie_state(dev);
 	pci_restore_pasid_state(dev);
@@ -3485,6 +3487,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
 	if (error)
 		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
 
+	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
+					    2 * sizeof(u32));
+	if (error)
+		pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
+
 	pci_allocate_vc_save_buffers(dev);
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 785f31086313..365a844ec430 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -561,10 +561,14 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pci_save_aspm_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
 #else
 static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
+static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
+static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
 #endif
 
 #ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ecbe3af4188d..dc2e21c7a9d4 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -721,6 +721,49 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 				PCI_L1SS_CTL1_L1SS_MASK, val);
 }
 
+void pci_save_aspm_l1ss_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u16 l1ss = dev->l1ss;
+	u32 *cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	if (!l1ss)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+	if (!save_state)
+		return;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	pci_read_config_dword(dev, l1ss + PCI_L1SS_CTL2, cap++);
+	pci_read_config_dword(dev, l1ss + PCI_L1SS_CTL1, cap++);
+}
+
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap, ctl1, ctl2;
+	u16 l1ss = dev->l1ss;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	if (!l1ss)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+	if (!save_state)
+		return;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	ctl2 = *cap++;
+	ctl1 = *cap;
+	aspm_program_l1ss(dev, ctl1, ctl2);
+}
+
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
 	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
-- 
2.17.1

Re: [PATCH V4 0/2] PCI/ASPM: Save/restore L1SS Capability for suspend/resume

[Bjorn Helgaas]

On Tue, Sep 13, 2022 at 06:48:20PM +0530, Vidya Sagar wrote:
> This patch series saves and restores the ASPM L1SS capability registers
> during suspend/resume cycle of the system.
> First patch refactors the existing L1SS register programming code to
> take out the common code.
> Second patch adds support to save and restore of the L1SS registers using
> the common code extracted in the first patch to restore the registers.
> 
> This patch is verified on Tegra194 and Tegra234 platforms.
> 
> Vidya Sagar (2):
>   PCI/ASPM: Refactor ASPM L1SS control register programming
>   PCI/ASPM: Save/restore L1SS Capability for suspend/resume
> 
>  drivers/pci/pci.c       |   7 +++
>  drivers/pci/pci.h       |   4 ++
>  drivers/pci/pcie/aspm.c | 106 +++++++++++++++++++++++++++-------------
>  3 files changed, 83 insertions(+), 34 deletions(-)

Applied to pci/aspm for v6.1, thanks!

I dropped the pci_is_pcie() testing since pci_configure_ltr() only
sets dev->l1ss for PCIe devices.

Re: [PATCH V4 1/2] PCI/ASPM: Refactor ASPM L1SS control register programming

[Bjorn Helgaas]

[+cc Krishna]

On Tue, Sep 13, 2022 at 06:48:21PM +0530, Vidya Sagar wrote:
> Refactor the code to extract the command code out to program
> Control Registers-1 & 2 of L1 Sub-States capability to a new function
> aspm_program_l1ss() and call it for both parent and child devices.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * New patch in this series
> 
>  drivers/pci/pcie/aspm.c | 63 +++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a8aec190986c..ecbe3af4188d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -455,6 +455,31 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
>  	pci_write_config_dword(pdev, pos, val);
>  }
>  
> +static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
> +{
> +	u16 l1ss = dev->l1ss;
> +	u32 l1_2_enable;
> +
> +	/*
> +	 * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
> +	 * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
> +	 */
> +	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
> +
> +	/*
> +	 * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
> +	 * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
> +	 * enable bits, even though they're all in PCI_L1SS_CTL1.
> +	 */
> +	l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> +	ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
> +
> +	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
> +	if (l1_2_enable)
> +		pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
> +				       ctl1 | l1_2_enable);
> +}
> +
>  /* Calculate L1.2 PM substate timing parameters */
>  static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  				u32 parent_l1ss_cap, u32 child_l1ss_cap)
> @@ -464,7 +489,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	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;
>  
>  	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>  		return;
> @@ -513,39 +537,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	    ctl2 == pctl2 && ctl2 == cctl2)
>  		return;
>  
> -	/* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
> -	pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> -	cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> -
> -	if (pl1_2_enables || cl1_2_enables) {
> -		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> -					PCI_L1SS_CTL1_L1_2_MASK, 0);
> -		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> -					PCI_L1SS_CTL1_L1_2_MASK, 0);
> -	}
> -
> -	/* Program T_POWER_ON times in both ports */
> -	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
> -	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
> -
> -	/* Program Common_Mode_Restore_Time in upstream device */
> -	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> -				PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
> -
> -	/* Program LTR_L1.2_THRESHOLD time in both ports */
> -	pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
> -				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> -				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
> -	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> -				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> -				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
> -
> -	if (pl1_2_enables || cl1_2_enables) {
> -		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
> -					pl1_2_enables);
> -		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
> -					cl1_2_enables);
> -	}
> +	aspm_program_l1ss(parent,
> +			  ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> +	aspm_program_l1ss(child,
> +			  ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);

This doesn't seem right to me.  I think the intent is to update
LTR_L1.2_THRESHOLD and Common_Mode_Restore_Time, which are encoded in
"ctl1".  It does do that, but it looks like it *also* clears
everything except PCI_L1SS_CTL1_L1_2_MASK, i.e., the L1.1 Enable bits,
the Link Activation bits, and the RsvdP bits, which I don't think we
should be clearing.  Am I missing something?

Bjorn

Re: [PATCH V4 1/2] PCI/ASPM: Refactor ASPM L1SS control register programming

[Vidya Sagar]

On 9/30/2022 3:30 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> [+cc Krishna]
> 
> On Tue, Sep 13, 2022 at 06:48:21PM +0530, Vidya Sagar wrote:
>> Refactor the code to extract the command code out to program
>> Control Registers-1 & 2 of L1 Sub-States capability to a new function
>> aspm_program_l1ss() and call it for both parent and child devices.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> V4:
>> * New patch in this series
>>
>>   drivers/pci/pcie/aspm.c | 63 +++++++++++++++++++----------------------
>>   1 file changed, 29 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index a8aec190986c..ecbe3af4188d 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -455,6 +455,31 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
>>        pci_write_config_dword(pdev, pos, val);
>>   }
>>
>> +static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
>> +{
>> +     u16 l1ss = dev->l1ss;
>> +     u32 l1_2_enable;
>> +
>> +     /*
>> +      * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
>> +      * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
>> +      */
>> +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
>> +
>> +     /*
>> +      * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
>> +      * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
>> +      * enable bits, even though they're all in PCI_L1SS_CTL1.
>> +      */
>> +     l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>> +     ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
>> +
>> +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
>> +     if (l1_2_enable)
>> +             pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
>> +                                    ctl1 | l1_2_enable);
>> +}
>> +
>>   /* Calculate L1.2 PM substate timing parameters */
>>   static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>                                u32 parent_l1ss_cap, u32 child_l1ss_cap)
>> @@ -464,7 +489,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>        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;
>>
>>        if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>>                return;
>> @@ -513,39 +537,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>            ctl2 == pctl2 && ctl2 == cctl2)
>>                return;
>>
>> -     /* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
>> -     pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>> -     cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>> -
>> -     if (pl1_2_enables || cl1_2_enables) {
>> -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
>> -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
>> -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>> -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
>> -     }
>> -
>> -     /* Program T_POWER_ON times in both ports */
>> -     pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
>> -     pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
>> -
>> -     /* Program Common_Mode_Restore_Time in upstream device */
>> -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>> -                             PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
>> -
>> -     /* Program LTR_L1.2_THRESHOLD time in both ports */
>> -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>> -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
>> -     pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
>> -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
>> -
>> -     if (pl1_2_enables || cl1_2_enables) {
>> -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
>> -                                     pl1_2_enables);
>> -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
>> -                                     cl1_2_enables);
>> -     }
>> +     aspm_program_l1ss(parent,
>> +                       ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
>> +     aspm_program_l1ss(child,
>> +                       ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> 
> This doesn't seem right to me.  I think the intent is to update
> LTR_L1.2_THRESHOLD and Common_Mode_Restore_Time, which are encoded in
> "ctl1".  It does do that, but it looks like it *also* clears
> everything except PCI_L1SS_CTL1_L1_2_MASK, i.e., the L1.1 Enable bits,
> the Link Activation bits, and the RsvdP bits, which I don't think we
> should be clearing.  Am I missing something?

Agree. Instead of updating some of the register fields with new values 
while keeping other fields intact, this code programs the register 
fields with only new values and also programming all other fields to 
zero which is wrong.
Thanks for catching this. I missed it as the card with which I had 
tested didn't have L1.1 support.
I think the following modification should fix this issue.


@@ -537,10 +537,23 @@ static void aspm_calc_l1ss_info(struct 
pcie_link_state *link,
             ctl2 == pctl2 && ctl2 == cctl2)
                 return;

-       aspm_program_l1ss(parent,
-                         ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
-       aspm_program_l1ss(child,
-                         ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
+       pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+       pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
+                );
+       aspm_program_l1ss(parent, pctl1, ctl2);
+
+       cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+       cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
+                );
+       aspm_program_l1ss(child, cctl1, ctl2);
  }

  static void pcie_aspm_cap_init(struct pcie_link_state *link, int 
blacklist)

But, given that you mentioned we shouldn't be touching Rsvd also, I'm 
wondering if the following can cause any issue?
With the top of the tree code, the CMRT in ctrl1 register is updated 
only for the upstream device whereas with my change, it gets updated 
even for the downstream device. Although spec says that it is Rsvd for a 
downstream device (i.e. upstream port), I'm wondering if we should 
really avoid touching it?
If the answer is yes, I think it is better to drop the modifications 
done to aspm_calc_l1ss_info() function and just proceed with rest of the 
modifications given the way it is differentiating between upstream and 
downstream devices while updating the registers.
What are your comments on this?

Thanks,
Vidya Sagar

> 
> Bjorn
> 

Re: [PATCH V4 1/2] PCI/ASPM: Refactor ASPM L1SS control register programming

[Bjorn Helgaas]

On Mon, Oct 03, 2022 at 12:12:22PM +0530, Vidya Sagar wrote:
> On 9/30/2022 3:30 AM, Bjorn Helgaas wrote:
> > On Tue, Sep 13, 2022 at 06:48:21PM +0530, Vidya Sagar wrote:
> > > Refactor the code to extract the command code out to program
> > > Control Registers-1 & 2 of L1 Sub-States capability to a new function
> > > aspm_program_l1ss() and call it for both parent and child devices.
> > > 
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > ---
> > > V4:
> > > * New patch in this series
> > > 
> > >   drivers/pci/pcie/aspm.c | 63 +++++++++++++++++++----------------------
> > >   1 file changed, 29 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > index a8aec190986c..ecbe3af4188d 100644
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -455,6 +455,31 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
> > >        pci_write_config_dword(pdev, pos, val);
> > >   }
> > > 
> > > +static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
> > > +{
> > > +     u16 l1ss = dev->l1ss;
> > > +     u32 l1_2_enable;
> > > +
> > > +     /*
> > > +      * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
> > > +      * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
> > > +      */
> > > +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
> > > +
> > > +     /*
> > > +      * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
> > > +      * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
> > > +      * enable bits, even though they're all in PCI_L1SS_CTL1.
> > > +      */
> > > +     l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> > > +     ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
> > > +
> > > +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
> > > +     if (l1_2_enable)
> > > +             pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
> > > +                                    ctl1 | l1_2_enable);
> > > +}
> > > +
> > >   /* Calculate L1.2 PM substate timing parameters */
> > >   static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> > >                                u32 parent_l1ss_cap, u32 child_l1ss_cap)
> > > @@ -464,7 +489,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> > >        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;
> > > 
> > >        if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
> > >                return;
> > > @@ -513,39 +537,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> > >            ctl2 == pctl2 && ctl2 == cctl2)
> > >                return;
> > > 
> > > -     /* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
> > > -     pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> > > -     cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
> > > -
> > > -     if (pl1_2_enables || cl1_2_enables) {
> > > -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> > > -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
> > > -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> > > -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
> > > -     }
> > > -
> > > -     /* Program T_POWER_ON times in both ports */
> > > -     pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
> > > -     pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
> > > -
> > > -     /* Program Common_Mode_Restore_Time in upstream device */
> > > -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> > > -                             PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
> > > -
> > > -     /* Program LTR_L1.2_THRESHOLD time in both ports */
> > > -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> > > -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> > > -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
> > > -     pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> > > -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> > > -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
> > > -
> > > -     if (pl1_2_enables || cl1_2_enables) {
> > > -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
> > > -                                     pl1_2_enables);
> > > -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
> > > -                                     cl1_2_enables);
> > > -     }
> > > +     aspm_program_l1ss(parent,
> > > +                       ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> > > +     aspm_program_l1ss(child,
> > > +                       ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> > 
> > This doesn't seem right to me.  I think the intent is to update
> > LTR_L1.2_THRESHOLD and Common_Mode_Restore_Time, which are encoded in
> > "ctl1".  It does do that, but it looks like it *also* clears
> > everything except PCI_L1SS_CTL1_L1_2_MASK, i.e., the L1.1 Enable bits,
> > the Link Activation bits, and the RsvdP bits, which I don't think we
> > should be clearing.  Am I missing something?
> 
> Agree. Instead of updating some of the register fields with new values while
> keeping other fields intact, this code programs the register fields with
> only new values and also programming all other fields to zero which is
> wrong.
> Thanks for catching this. I missed it as the card with which I had tested
> didn't have L1.1 support.

> I think the following modification should fix this issue.
> 
> 
> @@ -537,10 +537,23 @@ static void aspm_calc_l1ss_info(struct pcie_link_state
> *link,
>             ctl2 == pctl2 && ctl2 == cctl2)
>                 return;
> 
> -       aspm_program_l1ss(parent,
> -                         ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> -       aspm_program_l1ss(child,
> -                         ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> +       pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
> +       pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
> +                );
> +       aspm_program_l1ss(parent, pctl1, ctl2);
> +
> +       cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
> +       cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
> +                );
> +       aspm_program_l1ss(child, cctl1, ctl2);
>  }
> 
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> 
> But, given that you mentioned we shouldn't be touching Rsvd also, I'm
> wondering if the following can cause any issue?
> With the top of the tree code, the CMRT in ctrl1 register is updated only
> for the upstream device whereas with my change, it gets updated even for the
> downstream device. Although spec says that it is Rsvd for a downstream
> device (i.e. upstream port), I'm wondering if we should really avoid
> touching it?

I'm not sure it's worth bothering about at this point.  It feels a
little OCD right now.

> If the answer is yes, I think it is better to drop the modifications done to
> aspm_calc_l1ss_info() function and just proceed with rest of the
> modifications given the way it is differentiating between upstream and
> downstream devices while updating the registers.
> What are your comments on this?

I thought we had some indication that Common_Mode_Restore_Time and
LTR_L1.2_THRESHOLD should be programmed *before* setting the L1.2
enable bits, even though they're all in PCI_L1SS_CTL1.

The first place that comment appears is
https://lore.kernel.org/linux-pci/20220907210540.GA140988@bhelgaas/,
but I can't remember if it actually solved a problem or if it was just
more OCD reading of the spec, which says "This field must only be
modified when the ASPM L1.2 Enable bit is Clear".

I'm inclined to keep the aspm_program_l1ss() changes unless we think
they're risky, because I think it is a small step forward, at least in
terms of reducing the number of config accesses.

On pci/aspm, I currently have your v4 patches plus these tweaks:

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index dc2e21c7a9d4..016d222b07c7 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -537,10 +537,21 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	    ctl2 == pctl2 && ctl2 == cctl2)
 		return;
 
-	aspm_program_l1ss(parent,
-			  ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
-	aspm_program_l1ss(child,
-			  ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
+	pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+		   PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+		   PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+	pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+			  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+			  PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
+	aspm_program_l1ss(parent, pctl1, ctl2);
+
+	cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+		   PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+		   PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+	cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+			  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+			  PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
+	aspm_program_l1ss(child, cctl1, ctl2);
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
@@ -727,9 +738,6 @@ void pci_save_aspm_l1ss_state(struct pci_dev *dev)
 	u16 l1ss = dev->l1ss;
 	u32 *cap;
 
-	if (!pci_is_pcie(dev))
-		return;
-
 	if (!l1ss)
 		return;
 
@@ -748,9 +756,6 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
 	u32 *cap, ctl1, ctl2;
 	u16 l1ss = dev->l1ss;
 
-	if (!pci_is_pcie(dev))
-		return;
-
 	if (!l1ss)
 		return;
 

Re: [PATCH V4 1/2] PCI/ASPM: Refactor ASPM L1SS control register programming

[Vidya Sagar]

On 10/5/2022 8:05 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Oct 03, 2022 at 12:12:22PM +0530, Vidya Sagar wrote:
>> On 9/30/2022 3:30 AM, Bjorn Helgaas wrote:
>>> On Tue, Sep 13, 2022 at 06:48:21PM +0530, Vidya Sagar wrote:
>>>> Refactor the code to extract the command code out to program
>>>> Control Registers-1 & 2 of L1 Sub-States capability to a new function
>>>> aspm_program_l1ss() and call it for both parent and child devices.
>>>>
>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>> ---
>>>> V4:
>>>> * New patch in this series
>>>>
>>>>    drivers/pci/pcie/aspm.c | 63 +++++++++++++++++++----------------------
>>>>    1 file changed, 29 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>>> index a8aec190986c..ecbe3af4188d 100644
>>>> --- a/drivers/pci/pcie/aspm.c
>>>> +++ b/drivers/pci/pcie/aspm.c
>>>> @@ -455,6 +455,31 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
>>>>         pci_write_config_dword(pdev, pos, val);
>>>>    }
>>>>
>>>> +static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
>>>> +{
>>>> +     u16 l1ss = dev->l1ss;
>>>> +     u32 l1_2_enable;
>>>> +
>>>> +     /*
>>>> +      * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
>>>> +      * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
>>>> +      */
>>>> +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
>>>> +
>>>> +     /*
>>>> +      * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
>>>> +      * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
>>>> +      * enable bits, even though they're all in PCI_L1SS_CTL1.
>>>> +      */
>>>> +     l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>>>> +     ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
>>>> +
>>>> +     pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
>>>> +     if (l1_2_enable)
>>>> +             pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
>>>> +                                    ctl1 | l1_2_enable);
>>>> +}
>>>> +
>>>>    /* Calculate L1.2 PM substate timing parameters */
>>>>    static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>>                                 u32 parent_l1ss_cap, u32 child_l1ss_cap)
>>>> @@ -464,7 +489,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>>         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;
>>>>
>>>>         if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>>>>                 return;
>>>> @@ -513,39 +537,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>>>>             ctl2 == pctl2 && ctl2 == cctl2)
>>>>                 return;
>>>>
>>>> -     /* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
>>>> -     pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>>>> -     cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
>>>> -
>>>> -     if (pl1_2_enables || cl1_2_enables) {
>>>> -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
>>>> -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
>>>> -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>>>> -                                     PCI_L1SS_CTL1_L1_2_MASK, 0);
>>>> -     }
>>>> -
>>>> -     /* Program T_POWER_ON times in both ports */
>>>> -     pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
>>>> -     pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
>>>> -
>>>> -     /* Program Common_Mode_Restore_Time in upstream device */
>>>> -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>>>> -                             PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
>>>> -
>>>> -     /* Program LTR_L1.2_THRESHOLD time in both ports */
>>>> -     pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>>>> -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>>>> -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
>>>> -     pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
>>>> -                             PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>>>> -                             PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
>>>> -
>>>> -     if (pl1_2_enables || cl1_2_enables) {
>>>> -             pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
>>>> -                                     pl1_2_enables);
>>>> -             pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
>>>> -                                     cl1_2_enables);
>>>> -     }
>>>> +     aspm_program_l1ss(parent,
>>>> +                       ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
>>>> +     aspm_program_l1ss(child,
>>>> +                       ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
>>>
>>> This doesn't seem right to me.  I think the intent is to update
>>> LTR_L1.2_THRESHOLD and Common_Mode_Restore_Time, which are encoded in
>>> "ctl1".  It does do that, but it looks like it *also* clears
>>> everything except PCI_L1SS_CTL1_L1_2_MASK, i.e., the L1.1 Enable bits,
>>> the Link Activation bits, and the RsvdP bits, which I don't think we
>>> should be clearing.  Am I missing something?
>>
>> Agree. Instead of updating some of the register fields with new values while
>> keeping other fields intact, this code programs the register fields with
>> only new values and also programming all other fields to zero which is
>> wrong.
>> Thanks for catching this. I missed it as the card with which I had tested
>> didn't have L1.1 support.
> 
>> I think the following modification should fix this issue.
>>
>>
>> @@ -537,10 +537,23 @@ static void aspm_calc_l1ss_info(struct pcie_link_state
>> *link,
>>              ctl2 == pctl2 && ctl2 == cctl2)
>>                  return;
>>
>> -       aspm_program_l1ss(parent,
>> -                         ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
>> -       aspm_program_l1ss(child,
>> -                         ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
>> +       pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
>> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
>> +       pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
>> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
>> +                );
>> +       aspm_program_l1ss(parent, pctl1, ctl2);
>> +
>> +       cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
>> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
>> +       cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
>> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
>> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE)
>> +                );
>> +       aspm_program_l1ss(child, cctl1, ctl2);
>>   }
>>
>>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>
>> But, given that you mentioned we shouldn't be touching Rsvd also, I'm
>> wondering if the following can cause any issue?
>> With the top of the tree code, the CMRT in ctrl1 register is updated only
>> for the upstream device whereas with my change, it gets updated even for the
>> downstream device. Although spec says that it is Rsvd for a downstream
>> device (i.e. upstream port), I'm wondering if we should really avoid
>> touching it?
> 
> I'm not sure it's worth bothering about at this point.  It feels a
> little OCD right now.
> 
>> If the answer is yes, I think it is better to drop the modifications done to
>> aspm_calc_l1ss_info() function and just proceed with rest of the
>> modifications given the way it is differentiating between upstream and
>> downstream devices while updating the registers.
>> What are your comments on this?
> 
> I thought we had some indication that Common_Mode_Restore_Time and
> LTR_L1.2_THRESHOLD should be programmed *before* setting the L1.2
> enable bits, even though they're all in PCI_L1SS_CTL1.
> 
> The first place that comment appears is
> https://lore.kernel.org/linux-pci/20220907210540.GA140988@bhelgaas/,
> but I can't remember if it actually solved a problem or if it was just
> more OCD reading of the spec, which says "This field must only be
> modified when the ASPM L1.2 Enable bit is Clear".
> 
> I'm inclined to keep the aspm_program_l1ss() changes unless we think
> they're risky, because I think it is a small step forward, at least in
> terms of reducing the number of config accesses.
> 
> On pci/aspm, I currently have your v4 patches plus these tweaks:

Thanks Bjorn for pushing these modifications and I'm fine with the below 
changes.

> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index dc2e21c7a9d4..016d222b07c7 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -537,10 +537,21 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>              ctl2 == pctl2 && ctl2 == cctl2)
>                  return;
> 
> -       aspm_program_l1ss(parent,
> -                         ctl1 | (pctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> -       aspm_program_l1ss(child,
> -                         ctl1 | (cctl1 & PCI_L1SS_CTL1_L1_2_MASK), ctl2);
> +       pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
> +       pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
> +       aspm_program_l1ss(parent, pctl1, ctl2);
> +
> +       cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                  PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
> +       cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +                         PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
> +       aspm_program_l1ss(child, cctl1, ctl2);
>   }
> 
>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> @@ -727,9 +738,6 @@ void pci_save_aspm_l1ss_state(struct pci_dev *dev)
>          u16 l1ss = dev->l1ss;
>          u32 *cap;
> 
> -       if (!pci_is_pcie(dev))
> -               return;
> -
>          if (!l1ss)
>                  return;
> 
> @@ -748,9 +756,6 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
>          u32 *cap, ctl1, ctl2;
>          u16 l1ss = dev->l1ss;
> 
> -       if (!pci_is_pcie(dev))
> -               return;
> -
>          if (!l1ss)
>                  return;
>