lkml.org 
[lkml]   [2020]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
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)
> > > --

\
 
 \ /
  Last update: 2020-11-19 19:14    [W:0.110 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site