* [PATCH 1/2] PCI: Add save/restore of Precision Time Measurement capability
@ 2020-11-19 0:18 David E. Box
2020-11-19 0:18 ` [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend David E. Box
0 siblings, 1 reply; 6+ messages in thread
From: David E. Box @ 2020-11-19 0:18 UTC (permalink / raw)
To: bhelgaas, rafael, len.brown; +Cc: David E. Box, linux-pci, linux-kernel
The PCI subsystem does not currently save and restore the configuration
space for the Precision Time Measurement (PTM) PCIe extended capability
leading to the feature returning disabled on S3 resume. This has been
observed on Intel Coffee Lake desktops. Add save/restore of the PTM control
register. This saves the PTM Enable, Root Select, and Effective Granularity
bits.
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e578d34095e9..6fd4ae910a88 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1539,6 +1539,44 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
}
+static void pci_save_ptm_state(struct pci_dev *dev)
+{
+ int ptm;
+ struct pci_cap_saved_state *save_state;
+ u16 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+ if (!ptm)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+ if (!save_state) {
+ pci_err(dev, "no suspend buffer for PTM\n");
+ return;
+ }
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
+}
+
+static void pci_restore_ptm_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int ptm;
+ u16 *cap;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+ ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+ if (!save_state || !ptm)
+ return;
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
+}
+
/**
* pci_save_state - save the PCI configuration space of a device before
* suspending
@@ -1566,6 +1604,7 @@ int pci_save_state(struct pci_dev *dev)
pci_save_ltr_state(dev);
pci_save_dpc_state(dev);
pci_save_aer_state(dev);
+ pci_save_ptm_state(dev);
return pci_save_vc_state(dev);
}
EXPORT_SYMBOL(pci_save_state);
@@ -1677,6 +1716,7 @@ void pci_restore_state(struct pci_dev *dev)
pci_restore_vc_state(dev);
pci_restore_rebar_state(dev);
pci_restore_dpc_state(dev);
+ pci_restore_ptm_state(dev);
pci_aer_clear_status(dev);
pci_restore_aer_state(dev);
@@ -3332,6 +3372,10 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for PTM\n");
+
pci_allocate_vc_save_buffers(dev);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
2020-11-19 0:18 [PATCH 1/2] PCI: Add save/restore of Precision Time Measurement capability David E. Box
@ 2020-11-19 0:18 ` David E. Box
2020-11-19 12:01 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: David E. Box @ 2020-11-19 0:18 UTC (permalink / raw)
To: bhelgaas, rafael, len.brown; +Cc: David E. Box, linux-pci, linux-kernel
On Intel client platforms that support suspend-to-idle, like Ice Lake,
root ports that have Precision Time Management (PTM) enabled can prevent
the port from being fully power gated, causing higher power consumption
while suspended. To prevent this, after saving the PTM control register,
disable the feature. The feature will be returned to its previous state
during restore.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
Reported-by: Len Brown <len.brown@intel.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
drivers/pci/pci.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6fd4ae910a88..a2b40497d443 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -21,6 +21,7 @@
#include <linux/module.h>
#include <linux/spinlock.h>
#include <linux/string.h>
+#include <linux/suspend.h>
#include <linux/log2.h>
#include <linux/logic_pio.h>
#include <linux/pm_wakeup.h>
@@ -1543,7 +1544,7 @@ static void pci_save_ptm_state(struct pci_dev *dev)
{
int ptm;
struct pci_cap_saved_state *save_state;
- u16 *cap;
+ u16 *cap, ctrl;
if (!pci_is_pcie(dev))
return;
@@ -1560,6 +1561,17 @@ static void pci_save_ptm_state(struct pci_dev *dev)
cap = (u16 *)&save_state->cap.data[0];
pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
+
+ /*
+ * On Intel systems that support suspend-to-idle, additional
+ * power savings can be gained by disabling PTM on root ports,
+ * as this allows the port to enter a deeper pm state.
+ */
+ if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
+ pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+ ctrl = *cap & ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
+ pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
+ }
}
static void pci_restore_ptm_state(struct pci_dev *dev)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
2020-11-19 0:18 ` [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend David E. Box
@ 2020-11-19 12:01 ` Rafael J. Wysocki
2020-11-19 17:45 ` David E. Box
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2020-11-19 12:01 UTC (permalink / raw)
To: David E. Box
Cc: Bjorn Helgaas, Rafael J. Wysocki, Len Brown, Linux PCI,
Linux Kernel Mailing List
On Thu, Nov 19, 2020 at 1:17 AM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> On Intel client platforms that support suspend-to-idle, like Ice Lake,
> root ports that have Precision Time Management (PTM) enabled can prevent
> the port from being fully power gated, causing higher power consumption
> while suspended. To prevent this, after saving the PTM control register,
> disable the feature. The feature will be returned to its previous state
> during restore.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
> Reported-by: Len Brown <len.brown@intel.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> drivers/pci/pci.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6fd4ae910a88..a2b40497d443 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -21,6 +21,7 @@
> #include <linux/module.h>
> #include <linux/spinlock.h>
> #include <linux/string.h>
> +#include <linux/suspend.h>
> #include <linux/log2.h>
> #include <linux/logic_pio.h>
> #include <linux/pm_wakeup.h>
> @@ -1543,7 +1544,7 @@ static void pci_save_ptm_state(struct pci_dev *dev)
> {
> int ptm;
> struct pci_cap_saved_state *save_state;
> - u16 *cap;
> + u16 *cap, ctrl;
>
> if (!pci_is_pcie(dev))
> return;
> @@ -1560,6 +1561,17 @@ static void pci_save_ptm_state(struct pci_dev *dev)
>
> cap = (u16 *)&save_state->cap.data[0];
> pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
> +
> + /*
> + * On Intel systems that support suspend-to-idle, additional
> + * power savings can be gained by disabling PTM on root ports,
> + * as this allows the port to enter a deeper pm state.
I would say "There are systems (for example, ...) where the power
drawn while suspended can be significantly reduced by disabling PTM on
PCIe root ports, as this allows the port to enter a lower-power PM
state and the SoC to reach a lower-power idle state as a whole".
> + */
> + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
AFAICS the target sleep state doesn't matter here, so I'd skip the
check above, but otherwise it LGTM.
> + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> + ctrl = *cap & ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
> + pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
> + }
> }
>
> static void pci_restore_ptm_state(struct pci_dev *dev)
> --
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
2020-11-19 12:01 ` Rafael J. Wysocki
@ 2020-11-19 17:45 ` David E. Box
2020-11-19 18:13 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: David E. Box @ 2020-11-19 17:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, Len Brown, Linux PCI, Linux Kernel Mailing List
On Thu, 2020-11-19 at 13:01 +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 19, 2020 at 1:17 AM David E. Box
> <david.e.box@linux.intel.com> wrote:
> > On Intel client platforms that support suspend-to-idle, like Ice
> > Lake,
> > root ports that have Precision Time Management (PTM) enabled can
> > prevent
> > the port from being fully power gated, causing higher power
> > consumption
> > while suspended. To prevent this, after saving the PTM control
> > register,
> > disable the feature. The feature will be returned to its previous
> > state
> > during restore.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
> > Reported-by: Len Brown <len.brown@intel.com>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> > drivers/pci/pci.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6fd4ae910a88..a2b40497d443 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -21,6 +21,7 @@
> > #include <linux/module.h>
> > #include <linux/spinlock.h>
> > #include <linux/string.h>
> > +#include <linux/suspend.h>
> > #include <linux/log2.h>
> > #include <linux/logic_pio.h>
> > #include <linux/pm_wakeup.h>
> > @@ -1543,7 +1544,7 @@ static void pci_save_ptm_state(struct pci_dev
> > *dev)
> > {
> > int ptm;
> > struct pci_cap_saved_state *save_state;
> > - u16 *cap;
> > + u16 *cap, ctrl;
> >
> > if (!pci_is_pcie(dev))
> > return;
> > @@ -1560,6 +1561,17 @@ static void pci_save_ptm_state(struct
> > pci_dev *dev)
> >
> > cap = (u16 *)&save_state->cap.data[0];
> > pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
> > +
> > + /*
> > + * On Intel systems that support suspend-to-idle,
> > additional
> > + * power savings can be gained by disabling PTM on root
> > ports,
> > + * as this allows the port to enter a deeper pm state.
>
> I would say "There are systems (for example, ...) where the power
> drawn while suspended can be significantly reduced by disabling PTM
> on
> PCIe root ports, as this allows the port to enter a lower-power PM
> state and the SoC to reach a lower-power idle state as a whole".
Okay.
>
> > + */
> > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
>
> AFAICS the target sleep state doesn't matter here, so I'd skip the
> check above, but otherwise it LGTM.
The target sleep state doesn't matter so much but that it's suspending
does. pci_save_state() is called during probe for the root ports (and
many other pci devices - I'm curious as to why). So without this check
the capability gets disabled on boot.
>
> > + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > + ctrl = *cap & ~(PCI_PTM_CTRL_ENABLE |
> > PCI_PTM_CTRL_ROOT);
> > + pci_write_config_word(dev, ptm + PCI_PTM_CTRL,
> > ctrl);
> > + }
> > }
> >
> > static void pci_restore_ptm_state(struct pci_dev *dev)
> > --
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
2020-11-19 17:45 ` David E. Box
@ 2020-11-19 18:13 ` Rafael J. Wysocki
2020-11-19 19:38 ` David E. Box
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2020-11-19 18:13 UTC (permalink / raw)
To: David Box
Cc: Rafael J. Wysocki, Bjorn Helgaas, Len Brown, Linux PCI,
Linux Kernel Mailing List
On Thu, Nov 19, 2020 at 6:45 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> On Thu, 2020-11-19 at 13:01 +0100, Rafael J. Wysocki wrote:
> > On Thu, Nov 19, 2020 at 1:17 AM David E. Box
> > <david.e.box@linux.intel.com> wrote:
> > > On Intel client platforms that support suspend-to-idle, like Ice
> > > Lake,
> > > root ports that have Precision Time Management (PTM) enabled can
> > > prevent
> > > the port from being fully power gated, causing higher power
> > > consumption
> > > while suspended. To prevent this, after saving the PTM control
> > > register,
> > > disable the feature. The feature will be returned to its previous
> > > state
> > > during restore.
> > >
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
> > > Reported-by: Len Brown <len.brown@intel.com>
> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > > drivers/pci/pci.c | 14 +++++++++++++-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 6fd4ae910a88..a2b40497d443 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -21,6 +21,7 @@
> > > #include <linux/module.h>
> > > #include <linux/spinlock.h>
> > > #include <linux/string.h>
> > > +#include <linux/suspend.h>
> > > #include <linux/log2.h>
> > > #include <linux/logic_pio.h>
> > > #include <linux/pm_wakeup.h>
> > > @@ -1543,7 +1544,7 @@ static void pci_save_ptm_state(struct pci_dev
> > > *dev)
> > > {
> > > int ptm;
> > > struct pci_cap_saved_state *save_state;
> > > - u16 *cap;
> > > + u16 *cap, ctrl;
> > >
> > > if (!pci_is_pcie(dev))
> > > return;
> > > @@ -1560,6 +1561,17 @@ static void pci_save_ptm_state(struct
> > > pci_dev *dev)
> > >
> > > cap = (u16 *)&save_state->cap.data[0];
> > > pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
> > > +
> > > + /*
> > > + * On Intel systems that support suspend-to-idle,
> > > additional
> > > + * power savings can be gained by disabling PTM on root
> > > ports,
> > > + * as this allows the port to enter a deeper pm state.
> >
> > I would say "There are systems (for example, ...) where the power
> > drawn while suspended can be significantly reduced by disabling PTM
> > on
> > PCIe root ports, as this allows the port to enter a lower-power PM
> > state and the SoC to reach a lower-power idle state as a whole".
>
> Okay.
>
> >
> > > + */
> > > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
> >
> > AFAICS the target sleep state doesn't matter here, so I'd skip the
> > check above, but otherwise it LGTM.
>
> The target sleep state doesn't matter so much but that it's suspending
> does. pci_save_state() is called during probe for the root ports (and
> many other pci devices - I'm curious as to why).
I tend to forget about this, sorry.
> So without this check the capability gets disabled on boot.
>
So instead of calling this from here, why don't we invoke the code
below from pci_prepare_to_sleep() and pci_finish_runtime_suspend(),
before enabling wakeup (and it needs to be re-done on failures, eg. by
restoring the cap from the saved copy)?
> > > + pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > > + ctrl = *cap & ~(PCI_PTM_CTRL_ENABLE |
> > > PCI_PTM_CTRL_ROOT);
> > > + pci_write_config_word(dev, ptm + PCI_PTM_CTRL,
> > > ctrl);
> > > + }
> > > }
> > >
> > > static void pci_restore_ptm_state(struct pci_dev *dev)
> > > --
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
2020-11-19 18:13 ` Rafael J. Wysocki
@ 2020-11-19 19:38 ` David E. Box
0 siblings, 0 replies; 6+ messages in thread
From: David E. Box @ 2020-11-19 19:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, Len Brown, Linux PCI, Linux Kernel Mailing List
On Thu, 2020-11-19 at 19:13 +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 19, 2020 at 6:45 PM David E. Box
> <david.e.box@linux.intel.com> wrote:
> > On Thu, 2020-11-19 at 13:01 +0100, Rafael J. Wysocki wrote:
> > > On Thu, Nov 19, 2020 at 1:17 AM David E. Box
> > > <david.e.box@linux.intel.com> wrote:
...
> > > >
> > > > + */
> > > > + if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
> > >
> > > AFAICS the target sleep state doesn't matter here, so I'd skip
> > > the
> > > check above, but otherwise it LGTM.
> >
> > The target sleep state doesn't matter so much but that it's
> > suspending
> > does. pci_save_state() is called during probe for the root ports
> > (and
> > many other pci devices - I'm curious as to why).
>
> I tend to forget about this, sorry.
>
> > So without this check the capability gets disabled on boot.
> >
>
> So instead of calling this from here, why don't we invoke the code
> below from pci_prepare_to_sleep() and pci_finish_runtime_suspend(),
> before enabling wakeup (and it needs to be re-done on failures, eg.
> by
> restoring the cap from the saved copy)?
Ok.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-19 19:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 0:18 [PATCH 1/2] PCI: Add save/restore of Precision Time Measurement capability David E. Box
2020-11-19 0:18 ` [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend David E. Box
2020-11-19 12:01 ` Rafael J. Wysocki
2020-11-19 17:45 ` David E. Box
2020-11-19 18:13 ` Rafael J. Wysocki
2020-11-19 19:38 ` David E. Box
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.