Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend

From: David E. Box
Date: Thu Nov 19 2020 - 12:45:49 EST


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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > 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