| Message ID | 20200311045249.5200-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/PM: Skip link training delay for S3 resume | expand |
Hi, On Wed, Mar 11, 2020 at 12:52:49PM +0800, Kai-Heng Feng wrote: > Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the > PCIe spec") added a 1100ms delay on resume for bridges that don't > support Link Active Reporting. > > The commit also states that the delay can be skipped for S3, as the > firmware should already handled the case for us. Delay can be skipped if the firmware provides _DSM with function 8 implemented according to PCI firmwre spec 3.2 sec 4.6.8. > So let's skip the link training delay for S3, to save 1100ms resume > time. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pci/pci-driver.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 0454ca0e4e3f..3050375bad04 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) > pci_fixup_device(pci_fixup_resume_early, pci_dev); > pcie_pme_root_status_cleanup(pci_dev); > > - if (!skip_bus_pm && prev_state == PCI_D3cold) > + if (!skip_bus_pm && prev_state == PCI_D3cold > + && !pm_resume_via_firmware()) So this would need to check for the _DSM result as well. We do evaluate it in pci_acpi_optimize_delay() (drivers/pci/pci-acpi.c) and that ends up lowering ->d3cold_delay so maybe check that here. > pci_bridge_wait_for_secondary_bus(pci_dev); > > if (pci_has_legacy_pm_support(pci_dev)) > -- > 2.17.1
Hi, > On Mar 11, 2020, at 18:28, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi, > > On Wed, Mar 11, 2020 at 12:52:49PM +0800, Kai-Heng Feng wrote: >> Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the >> PCIe spec") added a 1100ms delay on resume for bridges that don't >> support Link Active Reporting. >> >> The commit also states that the delay can be skipped for S3, as the >> firmware should already handled the case for us. > > Delay can be skipped if the firmware provides _DSM with function 8 > implemented according to PCI firmwre spec 3.2 sec 4.6.8. As someone who doesn't have access to the PCI spec... Questions below. > >> So let's skip the link training delay for S3, to save 1100ms resume >> time. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/pci/pci-driver.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >> index 0454ca0e4e3f..3050375bad04 100644 >> --- a/drivers/pci/pci-driver.c >> +++ b/drivers/pci/pci-driver.c >> @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) >> pci_fixup_device(pci_fixup_resume_early, pci_dev); >> pcie_pme_root_status_cleanup(pci_dev); >> >> - if (!skip_bus_pm && prev_state == PCI_D3cold) >> + if (!skip_bus_pm && prev_state == PCI_D3cold >> + && !pm_resume_via_firmware()) > > So this would need to check for the _DSM result as well. We do evaluate > it in pci_acpi_optimize_delay() (drivers/pci/pci-acpi.c) and that ends > up lowering ->d3cold_delay so maybe check that here. Do we need to wait for d3cold_delay here? Or we can also skip that as long as pci_acpi_dsm_guid and FUNCTION_DELAY_DSM present? Kai-Heng > >> pci_bridge_wait_for_secondary_bus(pci_dev); >> >> if (pci_has_legacy_pm_support(pci_dev)) >> -- >> 2.17.1
On Thu, Mar 12, 2020 at 12:23:46PM +0800, Kai-Heng Feng wrote: > Hi, > > > On Mar 11, 2020, at 18:28, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > > Hi, > > > > On Wed, Mar 11, 2020 at 12:52:49PM +0800, Kai-Heng Feng wrote: > >> Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the > >> PCIe spec") added a 1100ms delay on resume for bridges that don't > >> support Link Active Reporting. > >> > >> The commit also states that the delay can be skipped for S3, as the > >> firmware should already handled the case for us. > > > > Delay can be skipped if the firmware provides _DSM with function 8 > > implemented according to PCI firmwre spec 3.2 sec 4.6.8. > > As someone who doesn't have access to the PCI spec... > Questions below. > > > > >> So let's skip the link training delay for S3, to save 1100ms resume > >> time. > >> > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >> --- > >> drivers/pci/pci-driver.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > >> index 0454ca0e4e3f..3050375bad04 100644 > >> --- a/drivers/pci/pci-driver.c > >> +++ b/drivers/pci/pci-driver.c > >> @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) > >> pci_fixup_device(pci_fixup_resume_early, pci_dev); > >> pcie_pme_root_status_cleanup(pci_dev); > >> > >> - if (!skip_bus_pm && prev_state == PCI_D3cold) > >> + if (!skip_bus_pm && prev_state == PCI_D3cold > >> + && !pm_resume_via_firmware()) > > > > So this would need to check for the _DSM result as well. We do evaluate > > it in pci_acpi_optimize_delay() (drivers/pci/pci-acpi.c) and that ends > > up lowering ->d3cold_delay so maybe check that here. > > Do we need to wait for d3cold_delay here? > Or we can also skip that as long as pci_acpi_dsm_guid and FUNCTION_DELAY_DSM present? Actually I think pci_bridge_wait_for_secondary_bus() already takes it into account. Have you checked if the BIOS has this _DSM implemented in the first place?
> On Mar 12, 2020, at 16:04, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Thu, Mar 12, 2020 at 12:23:46PM +0800, Kai-Heng Feng wrote: >> Hi, >> >>> On Mar 11, 2020, at 18:28, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: >>> >>> Hi, >>> >>> On Wed, Mar 11, 2020 at 12:52:49PM +0800, Kai-Heng Feng wrote: >>>> Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the >>>> PCIe spec") added a 1100ms delay on resume for bridges that don't >>>> support Link Active Reporting. >>>> >>>> The commit also states that the delay can be skipped for S3, as the >>>> firmware should already handled the case for us. >>> >>> Delay can be skipped if the firmware provides _DSM with function 8 >>> implemented according to PCI firmwre spec 3.2 sec 4.6.8. >> >> As someone who doesn't have access to the PCI spec... >> Questions below. >> >>> >>>> So let's skip the link training delay for S3, to save 1100ms resume >>>> time. >>>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> drivers/pci/pci-driver.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >>>> index 0454ca0e4e3f..3050375bad04 100644 >>>> --- a/drivers/pci/pci-driver.c >>>> +++ b/drivers/pci/pci-driver.c >>>> @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) >>>> pci_fixup_device(pci_fixup_resume_early, pci_dev); >>>> pcie_pme_root_status_cleanup(pci_dev); >>>> >>>> - if (!skip_bus_pm && prev_state == PCI_D3cold) >>>> + if (!skip_bus_pm && prev_state == PCI_D3cold >>>> + && !pm_resume_via_firmware()) >>> >>> So this would need to check for the _DSM result as well. We do evaluate >>> it in pci_acpi_optimize_delay() (drivers/pci/pci-acpi.c) and that ends >>> up lowering ->d3cold_delay so maybe check that here. >> >> Do we need to wait for d3cold_delay here? >> Or we can also skip that as long as pci_acpi_dsm_guid and FUNCTION_DELAY_DSM present? > > Actually I think pci_bridge_wait_for_secondary_bus() already takes it > into account. Have you checked if the BIOS has this _DSM implemented in > the first place? -[0000:00]-+-00.0 Intel Corporation Device 9b44 +-1c.0-[03-3b]----00.0-[04-3b]--+-00.0-[05]----00.0 Intel Corporation JHL7540 Thunderbolt 3 NHI [Titan Ridge 2C 2018] | +-01.0-[06-3a]-- | \-02.0-[3b]----00.0 Intel Corporation JHL7540 Thunderbolt 3 USB Controller [Titan Ridge 2C 2018] 00:1c.0 has _DSM implemented. How do I check for the Thunderbolt device? It doesn't seem to have a fixed _ADR so I don't know how to locate it in DSDT/SSDT table. Log with additional debug message: [ 948.813025] ACPI: EC: interrupt unblocked [ 948.925017] pcieport 0000:00:01.0: pcie_wait_for_link_delay sleep 1100ms [ 949.065466] pcieport 0000:04:00.0: pcie_wait_for_link_delay sleep 1100ms [ 949.065468] pcieport 0000:04:02.0: pcie_wait_for_link_delay sleep 1100ms 00:01.0 is the port for discrete graphics. Kai-Heng
On Thu, Mar 12, 2020 at 05:45:32PM +0800, Kai-Heng Feng wrote: > > > > On Mar 12, 2020, at 16:04, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > > On Thu, Mar 12, 2020 at 12:23:46PM +0800, Kai-Heng Feng wrote: > >> Hi, > >> > >>> On Mar 11, 2020, at 18:28, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > >>> > >>> Hi, > >>> > >>> On Wed, Mar 11, 2020 at 12:52:49PM +0800, Kai-Heng Feng wrote: > >>>> Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the > >>>> PCIe spec") added a 1100ms delay on resume for bridges that don't > >>>> support Link Active Reporting. > >>>> > >>>> The commit also states that the delay can be skipped for S3, as the > >>>> firmware should already handled the case for us. > >>> > >>> Delay can be skipped if the firmware provides _DSM with function 8 > >>> implemented according to PCI firmwre spec 3.2 sec 4.6.8. > >> > >> As someone who doesn't have access to the PCI spec... > >> Questions below. > >> > >>> > >>>> So let's skip the link training delay for S3, to save 1100ms resume > >>>> time. > >>>> > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >>>> --- > >>>> drivers/pci/pci-driver.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > >>>> index 0454ca0e4e3f..3050375bad04 100644 > >>>> --- a/drivers/pci/pci-driver.c > >>>> +++ b/drivers/pci/pci-driver.c > >>>> @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) > >>>> pci_fixup_device(pci_fixup_resume_early, pci_dev); > >>>> pcie_pme_root_status_cleanup(pci_dev); > >>>> > >>>> - if (!skip_bus_pm && prev_state == PCI_D3cold) > >>>> + if (!skip_bus_pm && prev_state == PCI_D3cold > >>>> + && !pm_resume_via_firmware()) > >>> > >>> So this would need to check for the _DSM result as well. We do evaluate > >>> it in pci_acpi_optimize_delay() (drivers/pci/pci-acpi.c) and that ends > >>> up lowering ->d3cold_delay so maybe check that here. > >> > >> Do we need to wait for d3cold_delay here? > >> Or we can also skip that as long as pci_acpi_dsm_guid and FUNCTION_DELAY_DSM present? > > > > Actually I think pci_bridge_wait_for_secondary_bus() already takes it > > into account. Have you checked if the BIOS has this _DSM implemented in > > the first place? > > -[0000:00]-+-00.0 Intel Corporation Device 9b44 > +-1c.0-[03-3b]----00.0-[04-3b]--+-00.0-[05]----00.0 Intel Corporation JHL7540 Thunderbolt 3 NHI [Titan Ridge 2C 2018] > | +-01.0-[06-3a]-- > | \-02.0-[3b]----00.0 Intel Corporation JHL7540 Thunderbolt 3 USB Controller [Titan Ridge 2C 2018] > > 00:1c.0 has _DSM implemented. > How do I check for the Thunderbolt device? Most likely you see it only under the root port (1c.0) so check /sys/bus/pci/devices/0000:00:1c.0/firmware_node/path and match that one to the ACPI tables. > It doesn't seem to have a fixed _ADR so I don't know how to locate it in DSDT/SSDT table. > > Log with additional debug message: > [ 948.813025] ACPI: EC: interrupt unblocked > [ 948.925017] pcieport 0000:00:01.0: pcie_wait_for_link_delay sleep 1100ms > [ 949.065466] pcieport 0000:04:00.0: pcie_wait_for_link_delay sleep 1100ms > [ 949.065468] pcieport 0000:04:02.0: pcie_wait_for_link_delay sleep 1100ms > > 00:01.0 is the port for discrete graphics. There is something wrong somewhere because the 1100ms sleep is totally not expected to happen, but I think this is the same issue that we discuss on another thread so let's use that thread instead to avoid confusion :)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 0454ca0e4e3f..3050375bad04 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -916,7 +916,8 @@ static int pci_pm_resume_noirq(struct device *dev) pci_fixup_device(pci_fixup_resume_early, pci_dev); pcie_pme_root_status_cleanup(pci_dev); - if (!skip_bus_pm && prev_state == PCI_D3cold) + if (!skip_bus_pm && prev_state == PCI_D3cold + && !pm_resume_via_firmware()) pci_bridge_wait_for_secondary_bus(pci_dev); if (pci_has_legacy_pm_support(pci_dev))
Commit ad9001f2f411 ("PCI/PM: Add missing link delays required by the PCIe spec") added a 1100ms delay on resume for bridges that don't support Link Active Reporting. The commit also states that the delay can be skipped for S3, as the firmware should already handled the case for us. So let's skip the link training delay for S3, to save 1100ms resume time. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pci/pci-driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)