Re: [PATCH] PCI: Quirk Intel DG2 ASPM L1 acceptable latency to be unlimited

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Tue, 2022-04-05 at 11:01 -0500, Bjorn Helgaas wrote:
> On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote:
> > Intel DG2 discrete graphics PCIe endpoints hard-code their
> > acceptable L1
> > ASPM latency to be < 1us even though the hardware actually supports
> > higher latencies (> 64 us) just fine. In order to allow the links
> > to go
> > into L1 and save power, quirk the acceptable L1 ASPM latency for
> > these
> > endpoints to be unlimited.
> 
> Is there a plan to fix this in future DG2 hardware/firmware?
> Obviously the point of Dev Cap is to make the device self-describing
> so we can avoid updates like this every time new hardware comes out.

Unfortunately no fix possible in DG2 hw (pci id list below), but it
will be fixed for other platforms coming after those.

> 
> > Note this does not have any effect unless the user requested the
> > kernel
> > to enable ASPM in the first place (by default we don't enable it). 
> 
> I think this depends on the platform and kernel config, doesn't it?
> If CONFIG_PCIEASPM_POWERSAVE=y or CONFIG_PCIEASPM_POWER_SUPERSAVE=y
> I suspect we would enable ASPM L1 even without the parameters below.
> 
> > This is done with "pcie_aspm=force
> > pcie_aspm.policy=powersupsersave"
> > command line parameters.
> 
> s/powersupsersave/powersupersave/
> 
> This should affect "powersave" as well as "powersupersave", right?
> Both enable L1.  "powersupersave" enables the L1 substates.
> 
> We *should* be able to enable/disable ASPM L1 using the sysfs
> "l1_aspm
> file, too.
> 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/quirks.c | 44
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index da829274fc66..e97b5daa00eb 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL,
> > 0x1533, rom_bar_overlap_defect);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536,
> > rom_bar_overlap_defect);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537,
> > rom_bar_overlap_defect);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538,
> > rom_bar_overlap_defect);
> > +
> > +#ifdef CONFIG_PCIEASPM
> > +/*
> > + * Intel DG2 graphics card has hard-coded acceptable L1 latency
> > that is
> > + * too low which prevents ASPM to be enabled. It does support ASPM
> > L1
> > + * and tolerates higher latencies so quirk it to be unlimited.
> > + */
> > +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev)
> > +{
> > +       if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) {
> > +               u32 devcap = dev->devcap;
> > +
> > +               dev->devcap |= 7 << 9;
> > +               pci_info(dev, "quirking devcap for L1 accepted
> > latency 0x%08x -> 0x%08x\n",
> > +                        devcap, dev->devcap);
> > +       }
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0,
> > quirk_aspm_accepted_l1_latency);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1,
> > quirk_aspm_accepted_l1_latency);
> > +#endif
> > -- 
> > 2.35.1
> >