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

[David E. Box]

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