All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: "Saheed O . Bolarinwa" <refactormyself@gmail.com>
Cc: Puranjay Mohan <puranjay12@gmail.com>,
	Rajat Jain <rajatja@google.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Yicong Yang <yangyicong@hisilicon.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss
Date: Fri, 16 Oct 2020 15:59:02 -0500	[thread overview]
Message-ID: <20201016205902.GA107669@bjorn-Precision-5520> (raw)
In-Reply-To: <20201015193039.12585-13-helgaas@kernel.org>

On Thu, Oct 15, 2020 at 02:30:39PM -0500, Bjorn Helgaas wrote:
> From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>
> 
> 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@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  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@gmail.com>
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@kernel.org
    Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

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

      reply	other threads:[~2020-10-16 20:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 02/12] PCI/ASPM: Move LTR path check to where it's used Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 03/12] PCI/ASPM: Use 'parent' and 'child' for readability Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 04/12] PCI/ASPM: Remove struct aspm_register_info.support Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 05/12] PCI/ASPM: Remove struct aspm_register_info.enabled Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 06/12] PCI/ASPM: Remove struct aspm_register_info.latency_encoding Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 07/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 08/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused) Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 09/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1 Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 10/12] PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 11/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss Bjorn Helgaas
2020-10-16 20:59   ` Bjorn Helgaas [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201016205902.GA107669@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=hkallweit1@gmail.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=puranjay12@gmail.com \
    --cc=rajatja@google.com \
    --cc=refactormyself@gmail.com \
    --cc=yangyicong@hisilicon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.