Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension

From: Victor Ding
Date: Thu Jan 14 2021 - 04:15:36 EST

Next message: Victor Ding: "Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored"
Previous message: Miguel Ojeda: "Re: [PATCH] Documentation/llvm: Add a section about supported architectures"
In reply to: Bjorn Helgaas: "Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jan 14, 2021 at 8:48 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Rafael, suspend/resume expert]
>
> On Wed, Jan 13, 2021 at 01:16:23PM +1100, Victor Ding wrote:
> > On Wed, Jan 13, 2021 at 9:38 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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?
>
> I think we should look at disabling ASPM for all devices during
> suspend. I really don't want to put this kind of gunk in individual
> drivers. If we *have* to put something in drivers, it should be using
> interfaces like pci_disable_link_state() instead of writing
> PCI_EXP_LNKCTL directly.
>
Agreed.
>
> I *would* be interested in more details about this issue you're seeing
> with GS9750, though. It will help the case for a core change if we
> can open a bugzilla.kernel.org issue with some of the details like the
> L1 exit latency (from "lspci -vv") and details of the activities that
> lead to these delays. Typical L1 exit latencies are <100us, so to see
> 200ms of delay would mean ~2000 L1.2 exits, which is higher than I
> would expect.
>
Sorry that I misread the PCIe specs and the trace results. It was
PortTPowerOnTime (Port T_POWER_ON) which added up to ~200ms.
GL9750's PortTPowerOnTime is 3100us or 3.1ms, and I saw up to 60
occurrences during resume.
I've created https://bugzilla.kernel.org/show_bug.cgi?id=211187
We can continue the discussion there.
>
> > > > Signed-off-by: Victor Ding <victording@xxxxxxxxxx>
> > > > ---
> > > >
> > > > 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
> > > >

Next message: Victor Ding: "Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored"
Previous message: Miguel Ojeda: "Re: [PATCH] Documentation/llvm: Add a section about supported architectures"
In reply to: Bjorn Helgaas: "Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]