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

[Ajay Agarwal <ajayagarwal@google.com>]

On Fri, Oct 04, 2024 at 06:19:28PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2024 at 06:55:03PM +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
> 
> I think we also write the L1.2 enable bits in PCI_L1SS_CTL1 in
> aspm_calc_l12_info() when ASPM L1 may be enabled:
> 
>   pcie_aspm_init_link_state
>     pcie_aspm_cap_init
>       pcie_capability_read_word(PCI_EXP_LNKCTL)
>       aspm_l1ss_init
>         aspm_calc_l12_info
>           pci_clear_and_set_config_dword(PCI_L1SS_CTL1, PCI_L1SS_CTL1_L1_2_MASK)
> 
> That looks like another path where we should make a similar change.
> What do you think?
>
I agree. We should make a similar change there. Thanks for pointing out.
Will make the change in the next version.
> Bjorn