Re: [PATCH 3/4 v3] PCI: s32g: Add initial PCIe support (RC)

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 14, 2025 at 11:05:45AM +0100, Christian Bruel wrote:
> > > The implication is that *every* user of dw_pcie_suspend_noirq() would
> > > have to check for the link being up.  There are only three existing
> > > callers:
> > > 
> > >    imx_pcie_suspend_noirq()
> > >    ls_pcie_suspend_noirq()
> > >    stm32_pcie_suspend_noirq()
> > > 
> > > but none of them checks for the link being up.
> 
> stm32 supports L1.1, so we bail out before pme_turn_off() in
> dw_pcie_suspend_noirq()

I think you're referring to this code::

  dw_pcie_suspend_noirq()
  {
        /*
         * If L1SS is supported, then do not put the link into L2 as some
         * devices such as NVMe expect low resume latency.
         */
        if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
                return 0;

        if (pci->pp.ops->pme_turn_off) {
                pci->pp.ops->pme_turn_off(&pci->pp);
        } else {
                ret = dw_pcie_pme_turn_off(pci);
                if (ret)
                        return ret;
        }

I think this is bogus.  For starters, the code doesn't match the
comment.  The comment talks about "L1SS being supported", but the code
checks for L1 (not L1SS).  And it checks whether L1 is *enabled* (in
Link Control), not whether it's *supported* (in Link Capabilities).

And it's up to the user whether L1 is enabled.  Users can disable L1
by building the kernel with PCIEASPM_PERFORMANCE, booting with
"pcie_aspm.policy=performance", or using sysfs.

It doesn't make sense to me to decide anything about PME_Turn_Off
based on PCI_EXP_LNKCTL_ASPM_L1.

We've had this conversation before, e.g.,
https://lore.kernel.org/linux-pci/?q=b%3Adw_pcie_suspend_noirq+b%3ANVMe+f%3Ahelgaas,
and Richard actually posted a patch to remove this code [2], but I
hesitated because I didn't think we had a good explanation for why it
was there in the first place and it was now OK to remove it.

But I think I was wrong and we should just remove this code and debug
whatever breaks.

> > If no devices are attached to the bus, then there is no need to
> > broadcast PME_Turn_Off and wait for L2/L3. I've just sent out a
> > series that fixes it [1].  Hopefully, this will allow Vincent to
> > use dw_pcie_{suspend/resume}_noirq() APIs.
> >
> > - Mani
> > 
> > [1] https://lore.kernel.org/linux-pci/20251106061326.8241-1-manivannan.sadhasivam@oss.qualcomm.com/
> 
> dw_pcie_suspend_noirq() path tested OK on stm32mp2.
> 
> Regards
> 
> Christian

[2] https://lore.kernel.org/linux-pci/20250924194457.GA2131297@bhelgaas/