Re: [PATCH] PCI: r8169: add suspend/resume aspm quirk

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 8 Jul 2024, George-Daniel Matei wrote:

> Added aspm suspend/resume hooks that run
> before and after suspend and resume to change
> the ASPM states of the PCI bus in order to allow
> the system suspend while trying to prevent card hangs
> 
> Signed-off-by: George-Daniel Matei <danielgeorgem@chromium.org>
> ---
>  drivers/pci/quirks.c | 142 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index dc12d4a06e21..aa3dba2211d3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6189,6 +6189,148 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, aspm_l1_acceptable_latency
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
> +
> +static const struct dmi_system_id chromebox_match_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Brask"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Aurash"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +		{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Bujia"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Gaelin"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Gladios"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Hahn"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Jeev"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Kinox"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Kuldax"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Lisbon"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{
> +			.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Moli"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		}
> +	},
> +	{ }
> +};
> +
> +static void rtl8169_suspend_aspm_settings(struct pci_dev *dev)
> +{
> +	u16 val = 0;
> +
> +	if (dmi_check_system(chromebox_match_table)) {
> +		//configure parent

Missing space.

> +		pcie_capability_clear_and_set_word(dev->bus->self,
> +						   PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_ASPMC,
> +						   PCI_EXP_LNKCTL_ASPM_L1);
> +
> +		pci_read_config_word(dev->bus->self,
> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				     &val);
> +		val = (val & ~PCI_L1SS_CTL1_L1SS_MASK) |
> +		      PCI_L1SS_CTL1_PCIPM_L1_2 | PCI_L1SS_CTL1_PCIPM_L1_2 |
> +		      PCI_L1SS_CTL1_ASPM_L1_1;
> +		pci_write_config_word(dev->bus->self,
> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				      val);

Touching ASPM state should be done through aspm driver, not by writing 
directly into LNKCTL and L1SS registers.

> +		//configure device

Missing space.

> +		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_ASPMC,
> +						   PCI_EXP_LNKCTL_ASPM_L1);
> +
> +		pci_read_config_word(dev, dev->l1ss + PCI_L1SS_CTL1, &val);
> +		val = (val & ~PCI_L1SS_CTL1_L1SS_MASK) |
> +		      PCI_L1SS_CTL1_PCIPM_L1_2 | PCI_L1SS_CTL1_PCIPM_L1_2 |
> +		      PCI_L1SS_CTL1_ASPM_L1_1;
> +		pci_write_config_word(dev, dev->l1ss + PCI_L1SS_CTL1, val);
> +	}
> +}
> +
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_REALTEK, 0x8168,
> +			  rtl8169_suspend_aspm_settings);
> +
> +static void rtl8169_resume_aspm_settings(struct pci_dev *dev)
> +{
> +	u16 val = 0;
> +
> +	if (dmi_check_system(chromebox_match_table)) {
> +		//configure device

Missing space

> +		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_ASPMC, 0);

pcie_capability_clear_word()

> +
> +		pci_read_config_word(dev->bus->self,

A copy-paste error for the device???

> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				     &val);
> +		val = val & ~PCI_L1SS_CTL1_L1SS_MASK;
> +		pci_write_config_word(dev->bus->self,
> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				      val);
> +
> +		//configure parent

Missing space

> +		pcie_capability_clear_and_set_word(dev->bus->self,
> +						   PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_ASPMC, 0);

pcie_capability_clear_and_set_word()

> +
> +		pci_read_config_word(dev->bus->self,
> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				     &val);
> +		val = val & ~PCI_L1SS_CTL1_L1SS_MASK;
> +		pci_write_config_word(dev->bus->self,
> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
> +				      val);

Touching the same device twice here?

> +	}
> +}
> +
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_REALTEK, 0x8168,
> +			 rtl8169_resume_aspm_settings);
>  #endif

-- 
 i.