From: "David E. Box" <david.e.box@linux.intel.com>
To: Jian-Hong Pan <jhp@endlessos.org>
Cc: "Bjorn Helgaas" <helgaas@kernel.org>,
"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: Tue, 30 Apr 2024 11:26:46 -0700 [thread overview]
Message-ID: <af74b8d4f1f7072ffebc8a7f5cf392140da73dc7.camel@linux.intel.com> (raw)
In-Reply-To: <CAPpJ_ecOah=gYfYJVX-TypRiK8+oons3rKOVOATb4epm6sGZaw@mail.gmail.com>
On Tue, 2024-04-30 at 15:46 +0800, Jian-Hong Pan wrote:
> David E. Box <david.e.box@linux.intel.com> 於 2024年4月27日 週六 上午8:03寫道：
> >
> > 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.
>
> Thanks for your review, but I notice some description in PCIe spec,
> 5.5.4 L1 PM Substates Configuration:
> "Prior to setting either or both of the enable bits for L1.2, the
> values for TPOWER_ON, Common_Mode_Restore_Time, and, if the ASPM L1.2
> Enable bit is to be Set, the LTR_L1.2_THRESHOLD (both Value and Scale
> fields) must be programmed." => I think this includes both "ASPM L1.2
> Enable" and "PCI-PM L1.2 Enable" bits.
That's fine. While the spec clearly calls out the ASPM L1.2 Enable bit here, I
see no harm in including PCI-PM L1.2 in that check. This is what the code
already does in aspm_l1ss_init().
The issue is the mixed used of two different types of flags that don't have the
same meaning. 'state' contains PCIE_LINK_STATE flags which are part of the
caller API to the pci_<enabled/disable>_link_state() functions. The ASPM_STATE
flags are used internally to aspm.c to track all states and their meaningful
combinations such as ASPM_STATE_L1_2_MASK which includes ASPM L1.2 and PCI-PM
L1.2. You should not do bit operations between them.
Also, you should not require that the timings be calculated only if L1_2 is
enabled. You should calculate the timings as long as it's capable. This is also
what aspm_l1ss_init() does.
The confusion might be over the fact that you are having
__pci_enable_link_state() call aspm_calc_l12_info(). This should have been
handled during initialization of the link in aspm_l1ss_init() and I'm not sure
why it didn't. Maybe it's because, for VMD, ASPM default state would have
started out all disabled and this somehow led to aspm_l1ss_init() not getting
called. But looking through the code I don't see it. It would be great if you
can confirm why they weren't calculated before.
David
>
> Jain-Hong Pan
>
> > '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-30 18:26 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
2024-04-30 7:46 ` Jian-Hong Pan
2024-04-30 18:26 ` David E. Box [this message]
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=af74b8d4f1f7072ffebc8a7f5cf392140da73dc7.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.