From: "David E. Box" <david.e.box@linux.intel.com>
To: Jian-Hong Pan <jhp@endlessos.org>, Bjorn Helgaas <helgaas@kernel.org>
Cc: "Johan Hovold" <johan@kernel.org>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Kuppuswamy Sathyanarayanan"
<sathyanarayanan.kuppuswamy@linux.intel.com>,
"Mika Westerberg" <mika.westerberg@linux.intel.com>,
"Damien Le Moal" <dlemoal@kernel.org>,
"Nirmal Patel" <nirmal.patel@linux.intel.com>,
"Jonathan Derrick" <jonathan.derrick@linux.dev>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state
Date: Fri, 26 Apr 2024 17:03:03 -0700 [thread overview]
Message-ID: <f111371300624b6f94f0746dbae66bd49f405eea.camel@linux.intel.com> (raw)
In-Reply-To: <20240424110223.21799-2-jhp@endlessos.org>
Hi Jian-Hong,
On Wed, 2024-04-24 at 19:02 +0800, Jian-Hong Pan wrote:
> Currently, when enable link's L1.2 features with __pci_enable_link_state(),
> it configs the link directly without ensuring related L1.2 parameters, such
> as T_POWER_ON, Common_Mode_Restore_Time, and LTR_L1.2_THRESHOLD have been
> programmed.
>
> This leads the link's L1.2 between PCIe Root Port and child device gets
> wrong configs when a caller tries to enabled it.
>
> Here is a failed example on ASUS B1400CEAE with enabled VMD:
>
> 10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe
> Controller (rev 01) (prog-if 00 [Normal decode])
> ...
> Capabilities: [200 v1] L1 PM Substates
> L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> L1_PM_Substates+
> PortCommonModeRestoreTime=45us PortTPowerOnTime=50us
> L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> T_CommonMode=45us LTR1.2_Threshold=101376ns
> L1SubCtl2: T_PwrOn=50us
>
> 10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe
> SSD (rev 01) (prog-if 02 [NVM Express])
> ...
> Capabilities: [900 v1] L1 PM Substates
> L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> L1_PM_Substates+
> PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
> L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> T_CommonMode=0us LTR1.2_Threshold=0ns
> L1SubCtl2: T_PwrOn=10us
>
> According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe
> Root Port and the child NVMe, they should be programmed with the same
> LTR1.2_Threshold value. However, they have different values in this case.
>
> Invoke aspm_calc_l12_info() to program the L1.2 parameters properly before
> enable L1.2 bits of L1 PM Substates Control Register in
> __pci_enable_link_state().
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> ---
> v2:
> - Prepare the PCIe LTR parameters before enable L1 Substates
>
> v3:
> - Only enable supported features for the L1 Substates part
>
> v4:
> - Focus on fixing L1.2 parameters, instead of re-initializing whole L1SS
>
> v5:
> - Fix typo and commit message
> - Split introducing aspm_get_l1ss_cap() to "PCI/ASPM: Introduce
> aspm_get_l1ss_cap()"
>
> drivers/pci/pcie/aspm.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c55ac11faa73..553327dee991 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1402,6 +1402,8 @@ EXPORT_SYMBOL(pci_disable_link_state);
> static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool
> locked)
> {
> struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> + struct pci_dev *child = link->downstream, *parent = link->pdev;
> + u32 parent_l1ss_cap, child_l1ss_cap;
>
> if (!link)
> return -EINVAL;
> @@ -1433,6 +1435,16 @@ static int __pci_enable_link_state(struct pci_dev
> *pdev, int state, bool locked)
> link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
> if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
> + /*
> + * Ensure L1.2 parameters: Common_Mode_Restore_Times, T_POWER_ON and
> + * LTR_L1.2_THRESHOLD are programmed properly before enable bits for
> + * L1.2, per PCIe r6.0, sec 5.5.4.
> + */
> + if (state & link->aspm_capable & ASPM_STATE_L1_2_MASK) {
This is still mixing PCIE_LINK_STATE flags with ASPM_STATE flags. 'state' should
not even matter. The timings should always be calculated and programmed as long
as L1_2 is capable. That way the timings are ready even if L1_2 isn't being
enabled now (in case the user enables it later).
David
> + parent_l1ss_cap = aspm_get_l1ss_cap(parent);
> + child_l1ss_cap = aspm_get_l1ss_cap(child);
> + aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
> + }
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
next prev parent reply other threads:[~2024-04-27 0:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 11:02 [PATCH v5 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state Jian-Hong Pan
2024-04-27 0:03 ` David E. Box [this message]
2024-04-30 7:46 ` Jian-Hong Pan
2024-04-30 18:26 ` David E. Box
2024-05-03 9:45 ` Jian-Hong Pan
2024-05-03 19:15 ` David E. Box
2024-05-03 22:28 ` Bjorn Helgaas
2024-05-13 10:40 ` Jian-Hong Pan
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=f111371300624b6f94f0746dbae66bd49f405eea.camel@linux.intel.com \
--to=david.e.box@linux.intel.com \
--cc=dlemoal@kernel.org \
--cc=helgaas@kernel.org \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jhp@endlessos.org \
--cc=johan@kernel.org \
--cc=jonathan.derrick@linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=nirmal.patel@linux.intel.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.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.