Re: [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 15, 2020 at 02:30:39PM -0500, Bjorn Helgaas wrote:
> From: "Saheed O. Bolarinwa" <refactormyself@xxxxxxxxx>
> 
> Previously we computed L1.2 parameters in the enumeration path, saved them
> in struct pcie_link_state.l1ss, and programmed them into the devices
> whenever we enabled or disabled L1.2 on the link.  But these parameters are
> constant and don't need to be updated when enabling/disabling L1.2.
> 
> Compute and program the L1.2 parameters once during enumeration and remove
> the struct pcie_link_state.l1ss member.  No functional change intended.
> 
> [bhelgaas: rework to program L1.2 parameters during enumeration]
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@xxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
>  drivers/pci/pcie/aspm.c | 55 +++++++++++++++--------------------------
>  1 file changed, 20 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index d76f23908d67..361eaa0c615d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -74,12 +74,6 @@ struct pcie_link_state {
>  	 * has one slot under it, so at most there are 8 functions.
>  	 */
>  	struct aspm_latency acceptable[8];
> -
> -	/* L1 PM Substate info */
> -	struct {
> -		u32 ctl1;		/* value to be programmed in ctl1 */
> -		u32 ctl2;		/* value to be programmed in ctl2 */
> -	} l1ss;
>  };
>  
>  static int aspm_disabled, aspm_force;
> @@ -461,8 +455,7 @@ 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 t_common_mode, t_power_on, l1_2_threshold, scale, value;
> -
> -	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
> +	u32 ctl1 = 0, ctl2 = 0;
>  
>  	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>  		return;
> @@ -480,10 +473,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  
>  	if (calc_l1ss_pwron(parent, scale1, val1) >
>  	    calc_l1ss_pwron(child, scale2, val2)) {
> -		link->l1ss.ctl2 |= scale1 | (val1 << 3);
> +		ctl2 |= scale1 | (val1 << 3);
>  		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
>  	} else {
> -		link->l1ss.ctl2 |= scale2 | (val2 << 3);
> +		ctl2 |= scale2 | (val2 << 3);
>  		t_power_on = calc_l1ss_pwron(child, scale2, val2);
>  	}
>  
> @@ -499,7 +492,23 @@ 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);
> -	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> +	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> +
> +	/* 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);

I think this is slightly problematic because L1.2 must be disabled
while updating T_POWER_ON, Common_Mode_Restore_Time, and
LTR_L1.2_THRESHOLD.  Updated patch to address this attached below.

>  }
>  
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> @@ -679,30 +688,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  						   PCI_EXP_LNKCTL_ASPM_L1, 0);
>  	}
>  
> -	if (enable_req & ASPM_STATE_L1_2_MASK) {
> -
> -		/* Program T_POWER_ON times in both ports */
> -		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
> -				       link->l1ss.ctl2);
> -		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
> -				       link->l1ss.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,
> -					link->l1ss.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,
> -					link->l1ss.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,
> -					link->l1ss.ctl1);
> -	}
> -
>  	val = 0;
>  	if (state & ASPM_STATE_L1_1)
>  		val |= PCI_L1SS_CTL1_ASPM_L1_1;
> -- 
> 2.25.1
> 

commit df8f10587d3d ("PCI/ASPM: Remove struct pcie_link_state.l1ss")
Author: Saheed O. Bolarinwa <refactormyself@xxxxxxxxx>
Date:   Thu Oct 15 14:30:39 2020 -0500

    PCI/ASPM: Remove struct pcie_link_state.l1ss
    
    Previously we computed L1.2 parameters in the enumeration path, saved them
    in struct pcie_link_state.l1ss, and programmed them into the devices
    whenever we enabled or disabled L1.2 on the link.  But these parameters are
    constant and don't need to be updated when enabling/disabling L1.2.
    
    Compute and program the L1.2 parameters once during enumeration and remove
    the struct pcie_link_state.l1ss member.  No functional change intended.
    
    [bhelgaas: rework to program L1.2 parameters during enumeration]
    Link: https://lore.kernel.org/r/20201015193039.12585-13-helgaas@xxxxxxxxxx
    Signed-off-by: Saheed O. Bolarinwa <refactormyself@xxxxxxxxx>
    Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d76f23908d67..ac0557a305af 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -74,12 +74,6 @@ struct pcie_link_state {
 	 * has one slot under it, so at most there are 8 functions.
 	 */
 	struct aspm_latency acceptable[8];
-
-	/* L1 PM Substate info */
-	struct {
-		u32 ctl1;		/* value to be programmed in ctl1 */
-		u32 ctl2;		/* value to be programmed in ctl2 */
-	} l1ss;
 };
 
 static int aspm_disabled, aspm_force;
@@ -461,8 +455,9 @@ 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 t_common_mode, t_power_on, l1_2_threshold, scale, value;
-
-	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
+	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;
@@ -480,10 +475,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 
 	if (calc_l1ss_pwron(parent, scale1, val1) >
 	    calc_l1ss_pwron(child, scale2, val2)) {
-		link->l1ss.ctl2 |= scale1 | (val1 << 3);
+		ctl2 |= scale1 | (val1 << 3);
 		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
 	} else {
-		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+		ctl2 |= scale2 | (val2 << 3);
 		t_power_on = calc_l1ss_pwron(child, scale2, val2);
 	}
 
@@ -499,7 +494,50 @@ 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);
-	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL2, &cctl2);
+
+	if (ctl1 == pctl1 && ctl1 == cctl1 &&
+	    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);
+	}
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
@@ -679,30 +717,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 						   PCI_EXP_LNKCTL_ASPM_L1, 0);
 	}
 
-	if (enable_req & ASPM_STATE_L1_2_MASK) {
-
-		/* Program T_POWER_ON times in both ports */
-		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
-		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.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,
-					link->l1ss.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,
-					link->l1ss.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,
-					link->l1ss.ctl1);
-	}
-
 	val = 0;
 	if (state & ASPM_STATE_L1_1)
 		val |= PCI_L1SS_CTL1_ASPM_L1_1;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 06846ec2e071..c7e0acba0e20 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1058,6 +1058,7 @@
 #define  PCI_L1SS_CTL1_PCIPM_L1_1	0x00000002  /* PCI-PM L1.1 Enable */
 #define  PCI_L1SS_CTL1_ASPM_L1_2	0x00000004  /* ASPM L1.2 Enable */
 #define  PCI_L1SS_CTL1_ASPM_L1_1	0x00000008  /* ASPM L1.1 Enable */
+#define  PCI_L1SS_CTL1_L1_2_MASK	0x00000005
 #define  PCI_L1SS_CTL1_L1SS_MASK	0x0000000f
 #define  PCI_L1SS_CTL1_CM_RESTORE_TIME	0x0000ff00  /* Common_Mode_Restore_Time */
 #define  PCI_L1SS_CTL1_LTR_L12_TH_VALUE	0x03ff0000  /* LTR_L1.2_THRESHOLD_Value */



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux