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: <20210112040146.2.Ic902bbd9f04e2d82ac578411e7fafc77b6c750e2@changeid>
 <20210112223830.GA1858627@bjorn-Precision-5520>
In-Reply-To: <20210112223830.GA1858627@bjorn-Precision-5520>
From:   Victor Ding <victording@google.com>
Date:   Wed, 13 Jan 2021 13:16:23 +1100
Message-ID: <CANqTbdYYSAvpzN2oaPSb2PUCE=rssj19GueTZmkvmukQWj9vUw@mail.gmail.com>
Subject: Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ben Chuang <ben.chuang@genesyslogic.com.tw>,
        LKML <linux-kernel@vger.kernel.org>, linux-pci@vger.kernel.org,
        linux-mmc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 13, 2021 at 9:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Jan 12, 2021 at 04:02:05AM +0000, Victor Ding wrote:
> > GL9750 has a 3100us PortTPowerOnTime; however, it enters L1.2 after
> > only ~4us inactivity per PCIe trace. During a suspend/resume process,
> > PCI access operations are frequently longer than 4us apart.
> > Therefore, the device frequently enters and leaves L1.2 during this
> > process, causing longer than desirable suspend/resume time. The total
> > time cost due to this L1.2 exit latency could add up to ~200ms.
> >
> > Considering that PCI access operations are fairly close to each other
> > (though sometimes > 4us), the actual time the device could stay in
> > L1.2 is negligible. Therefore, the little power-saving benefit from
> > ASPM during suspend/resume does not overweight the performance
> > degradation caused by long L1.2 exit latency.
> >
> > Therefore, this patch proposes to disable ASPM during a suspend/resume
> > process.
>
> This sounds like an interesting idea, but it doesn't seem like
> anything that's really device-dependent.  Drivers should not need to
> be involved in PCI configuration at this level, and they shouldn't
> read/write registers like PCI_EXP_LNKCTL directly.
>
> If we need to disable ASPM during suspend, I'd rather do it in the PCI
> core so all devices can benefit and drivers don't need to worry about
> it.
>
Good point. In theory all devices could encounter this issue, and it
more-likely occurs on those with low entry timer but high exit latency.
GL9750 is the only one I have access to that has such characteristics.

I think we should have ASPM disabled during suspend, or at least part
of the suspend process*, mainly for two reasons:
1. Power saving is expected to be little. During suspend/resume, we
    frequently access PCI registers, making it unlikely to stay in low
    power states;
2. It could cause performance degradation. Unfortunate timing could
    put the device into low power states and wake it up very soon after;
    resulting noticeable delays.
* By "part if the suspend process", I refer [suspend/resume]_noirq, where
there are frequent PCI register accesses and suffers most from this issue.

Ideally, the entry time could be tune so that it is long enough that the
device would not go into low power states during suspend; however, it
may not be feasible mainly because:
1. Hardware limitations;
2. The best timing for suspend/resume may not be the best timing for other
    tasks. Considering suspend/resume is a rare task, we probably do not
    want to sacrifice other tasks;
3. If the goal is to avoid entering low power states during suspend, it might
    be better just to disable it.

What do you think?
>
> > Signed-off-by: Victor Ding <victording@google.com>
> > ---
> >
> >  drivers/mmc/host/sdhci-pci-core.c |  2 +-
> >  drivers/mmc/host/sdhci-pci-gli.c  | 46 +++++++++++++++++++++++++++++--
> >  drivers/mmc/host/sdhci-pci.h      |  1 +
> >  3 files changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> > index 9552708846ca..fd7544a498c0 100644
> > --- a/drivers/mmc/host/sdhci-pci-core.c
> > +++ b/drivers/mmc/host/sdhci-pci-core.c
> > @@ -67,7 +67,7 @@ static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> >       return 0;
> >  }
> >
> > -static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> > +int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> >  {
> >       int i, ret;
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index 9887485a4134..c7b788b0e22e 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -109,6 +109,12 @@
> >
> >  #define GLI_MAX_TUNING_LOOP 40
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +struct gli_host {
> > +     u16 linkctl_saved;
> > +};
> > +#endif
> > +
> >  /* Genesys Logic chipset */
> >  static inline void gl9750_wt_on(struct sdhci_host *host)
> >  {
> > @@ -577,14 +583,48 @@ static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
> >  }
> >
> >  #ifdef CONFIG_PM_SLEEP
> > +static int sdhci_pci_gli_suspend(struct sdhci_pci_chip *chip)
> > +{
> > +     int ret;
> > +     struct sdhci_pci_slot *slot = chip->slots[0];
> > +     struct pci_dev *pdev = slot->chip->pdev;
> > +     struct gli_host *gli_host = sdhci_pci_priv(slot);
> > +
> > +     ret = pcie_capability_read_word(pdev, PCI_EXP_LNKCTL,
> > +                     &gli_host->linkctl_saved);
> > +     if (ret)
> > +             goto exit;
> > +
> > +     ret = pcie_capability_write_word(pdev, PCI_EXP_LNKCTL,
> > +                     gli_host->linkctl_saved & ~PCI_EXP_LNKCTL_ASPMC);
> > +     if (ret)
> > +             goto exit;
> > +
> > +     ret = sdhci_pci_suspend_host(chip);
> > +
> > +exit:
> > +     return ret;
> > +}
> > +
> >  static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip)
> >  {
> > +     int ret;
> >       struct sdhci_pci_slot *slot = chip->slots[0];
> > +     struct pci_dev *pdev = slot->chip->pdev;
> > +     struct gli_host *gli_host = sdhci_pci_priv(slot);
> >
> > -     pci_free_irq_vectors(slot->chip->pdev);
> > +     pci_free_irq_vectors(pdev);
> >       gli_pcie_enable_msi(slot);
> >
> > -     return sdhci_pci_resume_host(chip);
> > +     ret = sdhci_pci_resume_host(chip);
> > +     if (ret)
> > +             goto exit;
> > +
> > +     ret = pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
> > +                     PCI_EXP_LNKCTL_ASPMC, gli_host->linkctl_saved);
> > +
> > +exit:
> > +     return ret;
> >  }
> >
> >  static int sdhci_cqhci_gli_resume(struct sdhci_pci_chip *chip)
> > @@ -834,7 +874,9 @@ const struct sdhci_pci_fixes sdhci_gl9750 = {
> >       .probe_slot     = gli_probe_slot_gl9750,
> >       .ops            = &sdhci_gl9750_ops,
> >  #ifdef CONFIG_PM_SLEEP
> > +     .suspend        = sdhci_pci_gli_suspend,
> >       .resume         = sdhci_pci_gli_resume,
> > +     .priv_size      = sizeof(struct gli_host),
> >  #endif
> >  };
> >
> > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> > index d0ed232af0eb..16187a265e63 100644
> > --- a/drivers/mmc/host/sdhci-pci.h
> > +++ b/drivers/mmc/host/sdhci-pci.h
> > @@ -187,6 +187,7 @@ static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
> >  }
> >
> >  #ifdef CONFIG_PM_SLEEP
> > +int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip);
> >  int sdhci_pci_resume_host(struct sdhci_pci_chip *chip);
> >  #endif
> >  int sdhci_pci_enable_dma(struct sdhci_host *host);
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >