| Message ID | 20241001133519.2743673-1-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI/ASPM: Disable L1 before disabling L1ss | expand |
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. > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > --- > 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"? > @@ -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. 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. > /* 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 >
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 > ... > 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. Found some language about this in the ASPM Control description in PCIe r6.2, sec 7.5.3.7, Link Control. Also in sec 7.9.9.3, Root Complex Link Control, although I don't think Linux has any support for this register.
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@google.com> > > --- > > 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 > >
On Wed, Oct 02, 2024 at 03:09:26PM -0500, Bjorn Helgaas wrote: > 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 > > ... > > > 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. > > Found some language about this in the ASPM Control description in PCIe > r6.2, sec 7.5.3.7, Link Control. > Right. Added in the next version. > Also in sec 7.9.9.3, Root Complex Link Control, although I don't think > Linux has any support for this register. Right. There is no support for this reg.
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 @@ -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); + /* 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)
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 Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> --- drivers/pci/pcie/aspm.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)