On Thu, Dec 15, 2022 at 05:13:57PM +0800, Ron Lee wrote:
> On Google Coral and Reef family chromebooks, the PCIe bridge lost its
> L1 PM Substates capability after resumed from D3cold, and identify that
> the pointer to the this capability and capapability header are missing
> from the capability list.
s/chromebooks/Chromebooks/
s/to the this/this/
s/capapability/capability/
This should say what problem we're solving. I assume some devices
used L1 PM Substates before suspend, but after resume they do not, so
the user-visible effect is that battery life is worse after resume.
> Capabilities: [150 v0] Null
> Capabilities: [200 v1] L1 PM Substates
> L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ ...
> PortCommonModeRestoreTime=40us PortTPowerOnTime=10us
> L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> T_CommonMode=40us LTR1.2_Threshold=98304ns
> L1SubCtl2: T_PwrOn=60us
I'm not sure what this snippet is telling me. Based on the patch, I
guess before suspend, lspci would show:
Capabilities: [150 v0] Null
Capabilities: [200 v1] L1 PM Substates
Capabilities: [220] <some other valid capability?>
but after resume, you see only:
Capabilities: [150 v0] Null
Right?
> This patch fix up the header and the pointer to the L1SS capability
> after resuming from D3Cold.
The main problem here is that this patch covers up an issue without
saying what the root cause is. Presumably this is a firmware issue.
Has that been identified? Has it been fixed for future firmware
releases?
s/D3Cold/D3cold/ to match above.
Is there a bug report for this issue? Include the URL here.
Is there a bug report for the firmware?
> Signed-off-by: Ron Lee <ron.lee@xxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> ---
Nits:
- Use "Apollo Lake" to match Intel usage.
- Below the "---" line, mention what changed between v1 and v2 (I
see that you added the "#ifdef CONFIG_PCIEASPM", but you should
save readers the effort of figuring that out).
- For work-in-progress, the "Reported-by: kernel test robot" is
pointless and I will remove it. This quirk is not fixing a bug
reported by the robot.
> drivers/pci/quirks.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 285acc4aaccc..fc959be17a9d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5992,3 +5992,20 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
> #endif
> +
> +#ifdef CONFIG_PCIEASPM
> +static void chromeos_fixup_apl_bridge_l1ss_capability(struct pci_dev *pdev)
> +{
> + if (!dmi_match(DMI_SYS_VENDOR, "Google") ||
> + (!dmi_match(DMI_PRODUCT_FAMILY, "Google_Coral") &&
> + !dmi_match(DMI_PRODUCT_FAMILY, "Google_Reef")))
> + return;
> +
> + pci_info(pdev, "Fix up L1SS Capability\n");
> + /* Fix up the L1SS Capability Header*/
> + pci_write_config_dword(pdev, pdev->l1ss, (0x220 << 20) | (1 << 16) | (PCI_EXT_CAP_ID_L1SS));
This looks like it adds a link to another capability at offset 0x220.
What is that, and how do we know this is safe?
These registers are read-only per spec (PCIe r6.0, sec 7.8.3.1), but I
guess you have device-specific knowledge that they are writable?
> + /* Fix up the pointer to L1SS Capability*/
> + pci_write_config_dword(pdev, 0x150, pdev->l1ss << 20);
> +}
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_INTEL, 0x5ad6, chromeos_fixup_apl_bridge_l1ss_capability);
> +#endif
>
> base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770
> --
> 2.17.1
>
