Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201119001822.31617-1-david.e.box@linux.intel.com>
 <20201119001822.31617-2-david.e.box@linux.intel.com> <CAJZ5v0hGhyPySUdabwW5_LhyAKC3A4zdgj7H=55R=Xk3jvt3Yw@mail.gmail.com>
 <cdb520abba97ccf083788ed8ccb44fc042939468.camel@linux.intel.com>
In-Reply-To: <cdb520abba97ccf083788ed8ccb44fc042939468.camel@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 19 Nov 2020 19:13:21 +0100
Message-ID: <CAJZ5v0gyzYEiFWC4qvQZNDUC4wwcXK60mR=zJ9=Bwb27K1F=Ng@mail.gmail.com>
Subject: Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend
To:     David Box <david.e.box@linux.intel.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Len Brown <len.brown@intel.com>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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)
> > > --