RE: [PATCH v3] PCI: Fix up L1SS capability for Intel Apollo Lake PCIe bridge

[Lee, Ron]

> What do you think of the possible revision below?
> 
>   - Moved to arch/x86/pci/fixup.c since this is x86-only.
>   - Save prev cap offset & header, L1SS offset & header.  This means
>     we can fix up even when CONFIG_PCIEASPM is not enabled, we can
>     restore the entire previous cap header (not just the link), and
>     should be safe since only one device per system should match the
>     Device ID.
> 
> Bjorn
> 
Hi Bjorn,

Thank you revise this patch, 
it is more concise and make sense moving to arch/x86/pci/fixup.c
I corrected the following statement in the loop.
> +		prev_header = header;

BTW, I add "return" to stop traversal once L1SS capability was found, 
will submit the v4 patch later for you review.

+	while (pos) {
+		pci_read_config_dword(dev, pos, &header);
+		if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_L1SS) {
+			prev_cap = prev;
+			prev_header = pheader;
+			l1ss_cap = pos;
+			l1ss_header = header;
+			return;
+		}
+
+		prev = pos;
+		pheader = header;
+		pos = PCI_EXT_CAP_NEXT(header);
+	}

> commit e082cb8ab59f ("PCI: Fix up L1SS capability for Intel Apollo Lake Root
> Port") parent 52589007b243
> Author: Ron Lee <ron.lee.intel@xxxxxxxxx>
> Date:   Mon Apr 3 16:30:16 2023 -0500
> 
>     PCI: Fix up L1SS capability for Intel Apollo Lake Root Port
> 
>     On Google Coral and Reef family Chromebooks with Intel Apollo Lake
>     SoC, firmware clobbers the headers of the L1 PM Substates capability and
>     the previous capability when returning from D3cold to D0.
> 
>     Save those headers at enumeration-time and restore them at resume.
> 
>     Link: https://lore.kernel.org/linux-pci/CAFJ_xbq0cxcH-
> cgpXLU4Mjk30+muWyWm1aUZGK7iG53yaLBaQg@xxxxxxxxxxxxxx/T/#u
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c index
> 615a76d70019..ad0dfb22b4a6 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -824,3 +824,61 @@ static void rs690_fix_64bit_dma(struct pci_dev *pdev)
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7910,
> rs690_fix_64bit_dma);
> 
>  #endif
> +
> +/*
> + * When returning from D3cold to D0, firmware on some Google Coral and
> +Reef
> + * family Chromebooks with Intel Apollo Lake SoC clobbers the headers
> +of
> + * both the L1 PM Substates capability and the previous capability for
> +the
> + * "Celeron N3350/Pentium N4200/Atom E3900 Series PCI Express Port B #1".
> + *
> + * Save those values at enumeration-time and restore them at resume.
> + */
> +
> +static u16 prev_cap, l1ss_cap;
> +static u32 prev_header, l1ss_header;
> +
> +static void chromeos_save_apl_pci_l1ss_capability(struct pci_dev *dev)
> +{
> +	int pos = PCI_CFG_SPACE_SIZE, prev = 0;
> +	u32 header, pheader = 0;
> +
> +	while (pos) {
> +		pci_read_config_dword(dev, pos, &header);
> +		if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_L1SS) {
> +			prev_cap = prev;
> +			prev_header = pheader;
> +			l1ss_cap = pos;
> +			l1ss_header = header;
> +		}
> +
> +		prev = pos;
> +		prev_header = header;
> +		pos = PCI_EXT_CAP_NEXT(header);
> +	}
> +}
> +
> +static void chromeos_fixup_apl_pci_l1ss_capability(struct pci_dev *dev)
> +{
> +	u32 header;
> +
> +	if (!prev_cap || !prev_header || !l1ss_cap || !l1ss_header)
> +		return;
> +
> +	/* Fixup the header of L1SS Capability if missing */
> +	pci_read_config_dword(dev, l1ss_cap, &header);
> +	if (header != l1ss_header) {
> +		pci_write_config_dword(dev, l1ss_cap, l1ss_header);
> +		pci_info(dev, "restore L1SS Capability header (was %#010x now
> %#010x)\n",
> +			 header, l1ss_header);
> +	}
> +
> +	/* Fixup the link to L1SS Capability if missing */
> +	pci_read_config_dword(dev, prev_cap, &header);
> +	if (header != prev_header) {
> +		pci_write_config_dword(dev, prev_cap, prev_header);
> +		pci_info(dev, "restore previous Capability header (was %#010x
> now %#010x)\n",
> +			 header, prev_header);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x5ad6,
> +chromeos_save_apl_pci_l1ss_capability);
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_INTEL, 0x5ad6,
> +chromeos_fixup_apl_pci_l1ss_capability);