On Wed, Oct 02, 2024 at 01:12:23PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 01, 2024 at 07:05:18PM +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
> >
> > 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
>
> This seems reasonable to me. If you have seen a failure that is fixed
> by this change, please include some details here.
>
The failure is seen when the RC CPU attempts to clear the RC L1ss
register after clearing the EP L1ss register. We do not clearly
understand the state machine on the RC side which leads to this
behavior, but it looks like RC attempts to enter L1ss again and at the
same time, access to RC L1ss register fails because aux clk is still not
active. I will add some details in the next version.
> > Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
> > ---
> > drivers/pci/pcie/aspm.c | 22 ++++++++--------------
> > 1 file changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index cee2365e54b8..d37f66f9e9c8 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -848,16 +848,14 @@ 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:
> > * - 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
> > + * - When enabling/disabling ASPM L1.x, need to disable L1
> > * (at child followed by parent).
> > * - The ASPM/PCIPM L1.2 must be disabled while programming timing
> > * parameters
>
> Since you're updating this comment already, can you add the spec
> citation here, e.g., "PCIe r6.2, sec 5.5.4"?
>
Added in the next version.
> > @@ -866,21 +864,17 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> > * what is needed.
> > */
> >
> > + /* Disable L1, and it gets enabled later in pcie_config_aspm_link() */
> > + 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);
>
> I think it would be nice to have the disable and enable in the same
> place. Could we move this to pcie_config_aspm_link()? I'm not sure
> the pcie_config_aspm_dev() wrapper adds a lot of value, but we could
> at least do both ends using the same wrapper.
>
Added in the next version.
> pcie_config_aspm_link() configures all children; here we only do one
> child. I suspect we should do disable L1 for all of them here, and
> doing both in pcie_config_aspm_link() would make that clearer.
>
> pcie_config_aspm_link() has a comment ("Spec 2.0 ...") about the
> configuration order, but I'd like to update that, add a section
> reference, and make sure we do the disable in the right order.
>
Fixed in the next version.
> > /* Disable all L1 substates */
> > pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
> > 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)
> > --
> > 2.46.1.824.gd892dcdcdd-goog
> >
