Re: [PATCH v3] PCI/ASPM: Disable L1 before disabling L1ss

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Oct 07, 2024 at 08:59:17AM +0530, Ajay Agarwal wrote:
> The current sequence in the driver for L1ss update is as follows.
> 
> Disable L1ss
> Disable L1
> Enable L1ss as required
> Enable L1 if required
> 
> With this sequence, a bus hang is observed during the L1ss
> disable sequence when the RC CPU attempts to clear the RC L1ss
> register after clearing the EP L1ss register. It looks like the
> RC attempts to enter L1ss again and at the same time, access to
> RC L1ss register fails because aux clk is still not active.
> 
> PCIe spec r6.2, section 5.5.4, recommends that setting either
> or both of the enable bits for ASPM L1 PM Substates must be done
> while ASPM L1 is disabled. My interpretation here is that
> clearing L1ss should also be done when L1 is disabled. Thereby,
> change the sequence as follows.
> 
> Disable L1
> Disable L1ss
> Enable L1ss as required
> Enable L1 if required
> 
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>

Applied to pci/aspm for v6.13, thank you, Ajay!

> ---
> Changelog since v2:
>  - Add the logic in pcie_aspm_cap_init()
> 
> Changelog since v1:
>  - Move the L1 disable/enable logic to pcie_config_aspm_link()
>  - Add detailed comments including spec version and section number
> 
>  drivers/pci/pcie/aspm.c | 66 +++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8..50997e378631 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -805,6 +805,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
>  	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
>  
> +	/* Make sure L0s/L1 are disabled before updating L1SS config */
> +	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
> +	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> +		pcie_capability_write_word(child, PCI_EXP_LNKCTL,
> +					   child_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
> +		pcie_capability_write_word(parent, PCI_EXP_LNKCTL,
> +					   parent_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
> +	}
> +
>  	/*
>  	 * Setup L0s state
>  	 *
> @@ -829,6 +838,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  
>  	aspm_l1ss_init(link);
>  
> +	/* Restore L0s/L1 if they were enabled */
> +	if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
> +	    FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> +		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, parent_lnkctl);
> +		pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
> +	}
> +
>  	/* Save default state */
>  	link->aspm_default = link->aspm_enabled;
>  
> @@ -848,17 +864,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  /* Configure the ASPM L1 substates */
>  static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  {
> -	u32 val, enable_req;
> +	u32 val;
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>  
> -	enable_req = (link->aspm_enabled ^ state) & state;
> -
>  	/*
> -	 * Here are the rules specified in the PCIe spec for enabling L1SS:
> +	 * Spec r6.2, section 5.5.4, mentions the rules for enabling L1SS:
>  	 * - When enabling L1.x, enable bit at parent first, then at child
>  	 * - When disabling L1.x, disable bit at child first, then at parent
> -	 * - When enabling ASPM L1.x, need to disable L1
> -	 *   (at child followed by parent).
>  	 * - The ASPM/PCIPM L1.2 must be disabled while programming timing
>  	 *   parameters
>  	 *
> @@ -871,16 +883,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  				       PCI_L1SS_CTL1_L1SS_MASK, 0);
>  	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>  				       PCI_L1SS_CTL1_L1SS_MASK, 0);
> -	/*
> -	 * If needed, disable L1, and it gets enabled later
> -	 * in pcie_config_aspm_link().
> -	 */
> -	if (enable_req & (PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1_2)) {
> -		pcie_capability_clear_word(child, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_ASPM_L1);
> -		pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_ASPM_L1);
> -	}
>  
>  	val = 0;
>  	if (state & PCIE_LINK_STATE_L1_1)
> @@ -937,21 +939,33 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
>  	}
>  
> +	/*
> +	 * Spec r6.2, section 5.5.4, recommends that setting either or both of
> +	 * the enable bits for ASPM L1 PM Substates must be done while ASPM L1
> +	 * is disabled. So disable L1 here, and it gets enabled later after the
> +	 * L1ss configuration has been completed.
> +	 *
> +	 * Spec r6.2, section 7.5.3.7, mentions that ASPM L1 must be enabled by
> +	 * software in the Upstream component on a Link prior to enabling ASPM
> +	 * L1 in the Downstream component on the Link. When disabling L1,
> +	 * software must disable ASPM L1 in the Downstream component on a Link
> +	 * prior to disabling ASPM L1 in the Upstream component on that Link.
> +	 *
> +	 * Spec doesn't mention L0s.
> +	 *
> +	 * Disable L1 and L0s here, and they get enabled later after the L1ss
> +	 * configuration has been completed.
> +	 */
> +	list_for_each_entry(child, &linkbus->devices, bus_list)
> +		pcie_config_aspm_dev(child, 0);
> +	pcie_config_aspm_dev(parent, 0);
> +
>  	if (link->aspm_capable & PCIE_LINK_STATE_L1SS)
>  		pcie_config_aspm_l1ss(link, state);
>  
> -	/*
> -	 * Spec 2.0 suggests all functions should be configured the
> -	 * same setting for ASPM. Enabling ASPM L1 should be done in
> -	 * upstream component first and then downstream, and vice
> -	 * versa for disabling ASPM L1. Spec doesn't mention L0S.
> -	 */
> -	if (state & PCIE_LINK_STATE_L1)
> -		pcie_config_aspm_dev(parent, upstream);
> +	pcie_config_aspm_dev(parent, upstream);
>  	list_for_each_entry(child, &linkbus->devices, bus_list)
>  		pcie_config_aspm_dev(child, dwstream);
> -	if (!(state & PCIE_LINK_STATE_L1))
> -		pcie_config_aspm_dev(parent, upstream);
>  
>  	link->aspm_enabled = state;
>  
> -- 
> 2.47.0.rc0.187.ge670bccf7e-goog
>