Re: [PATCH] PCI/ASPM: Enable ASPM on external PCIe devices

[Kai-Heng Feng]

On Fri, Jun 16, 2023 at 1:07 AM Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 6/15/23 12:04 AM, Kai-Heng Feng wrote:
> > When a PCIe device is hotplugged to a Thunderbolt port, ASPM is not
> > enabled for that device. However, when the device is plugged preboot,
> > ASPM is enabled by default.
> >
> > The disparity happens because BIOS doesn't have the ability to program
> > ASPM on hotplugged devices.
> >
> > So enable ASPM by default for external connected PCIe devices so ASPM
> > settings are consitent between preboot and hotplugged.
>
> Why it has to be consistent? Can you add info about what it solves?

It enables ASPM when BIOS can't program LNKCTL.

Enabling ASPM can bring significant power saving for modern CPUs.

It's also quite nice that it can make the BadDLLP caused by I225 device go away.

Kai-Heng

>
> >
> > On HP Thunderbolt Dock G4, enable ASPM can also fix BadDLLP error:
> > pcieport 0000:00:1d.0: AER: Corrected error received: 0000:07:04.0
> > pcieport 0000:07:04.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
> > pcieport 0000:07:04.0:   device [8086:0b26] error status/mask=00000080/00002000
> > pcieport 0000:07:04.0:    [ 7] BadDLLP
> >
> > The root cause is still unclear, but quite likely because the I225 on
> > the dock supports PTM, where ASPM timing is precalculated for the PTM.
> >
> > Cc: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217557
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> >  drivers/pci/pcie/aspm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 66d7514ca111..613b0754c9bb 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -119,7 +119,9 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
> >               /* Enable Everything */
> >               return ASPM_STATE_ALL;
> >       case POLICY_DEFAULT:
> > -             return link->aspm_default;
> > +             return dev_is_removable(&link->downstream->dev) ?
> > +                     link->aspm_capable :
> > +                     link->aspm_default;
> >       }
> >       return 0;
> >  }
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer