GL9750 SD Host Controller 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, I am proposing to disable ASPM during a suspend/resume process.
Victor Ding (2):
PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored
mmc: sdhci-pci-gli: Disable ASPM during a suspension
drivers/mmc/host/sdhci-pci-core.c | 2 +-
drivers/mmc/host/sdhci-pci-gli.c | 46 +++++++++++++++++++++++++++++--
drivers/mmc/host/sdhci-pci.h | 1 +
drivers/pci/pci.c | 11 ++++++++
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/aspm.c | 2 +-
6 files changed, 60 insertions(+), 4 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7-goog
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.
Signed-off-by: Victor Ding <[email protected]>
---
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
Right after powering up, the device may have ASPM enabled; however,
its LTR and/or L1ss controls may not be in the desired states; hence,
the device may enter L1.2 undesirably and cause resume performance
penalty. This is especially problematic if ASPM related control
registers are modified before a suspension.
Therefore, ASPM should disabled until its LTR and L1ss states are
fully restored.
Signed-off-by: Victor Ding <[email protected]>
---
drivers/pci/pci.c | 11 +++++++++++
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/aspm.c | 2 +-
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index eb323af34f1e..428de433f2e6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1660,6 +1660,17 @@ void pci_restore_state(struct pci_dev *dev)
if (!dev->state_saved)
return;
+ /*
+ * Right after powering up, the device may have ASPM enabled;
+ * however, its LTR and/or L1ss controls may not be in the desired
+ * states; as a result, the device may enter L1.2 undesirably and
+ * cause resume performance penalty.
+ * Therefore, ASPM is disabled until its LTR and L1ss states are
+ * fully restored.
+ * (enabling ASPM is part of pci_restore_pcie_state)
+ */
+ pcie_config_aspm_dev(dev, 0);
+
/*
* Restore max latencies (in the LTR capability) before enabling
* LTR itself (in the PCIe capability).
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e9ea5dfaa3e0..f774bd6d2555 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -564,6 +564,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
void pcie_aspm_exit_link_state(struct pci_dev *pdev);
void pcie_aspm_pm_state_change(struct pci_dev *pdev);
void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val);
void pci_save_aspm_l1ss_state(struct pci_dev *dev);
void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
#else
@@ -571,6 +572,7 @@ static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
+static inline void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) { }
static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
#endif
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a08e7d6dc248..45535b4e1595 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -778,7 +778,7 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
}
-static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
+void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
{
pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
PCI_EXP_LNKCTL_ASPMC, val);
--
2.30.0.284.gd98b1dd5eaa7-goog
Hi Victor,
Thanks for the patch. Improving suspend/resume performance is always
a good thing!
On Tue, Jan 12, 2021 at 04:02:04AM +0000, Victor Ding wrote:
> Right after powering up, the device may have ASPM enabled; however,
> its LTR and/or L1ss controls may not be in the desired states; hence,
> the device may enter L1.2 undesirably and cause resume performance
> penalty. This is especially problematic if ASPM related control
> registers are modified before a suspension.
s/L1ss/L1SS/ (several occurrences) to match existing usage.
> Therefore, ASPM should disabled until its LTR and L1ss states are
> fully restored.
>
> Signed-off-by: Victor Ding <[email protected]>
> ---
>
> drivers/pci/pci.c | 11 +++++++++++
> drivers/pci/pci.h | 2 ++
> drivers/pci/pcie/aspm.c | 2 +-
> 3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eb323af34f1e..428de433f2e6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1660,6 +1660,17 @@ void pci_restore_state(struct pci_dev *dev)
> if (!dev->state_saved)
> return;
>
> + /*
> + * Right after powering up, the device may have ASPM enabled;
I think this could be stated more precisely. "Right after powering
up," ASPM should be *disabled* because ASPM Control in the Link
Control register powers up as zero (disabled).
> + * however, its LTR and/or L1ss controls may not be in the desired
> + * states; as a result, the device may enter L1.2 undesirably and
> + * cause resume performance penalty.
> + * Therefore, ASPM is disabled until its LTR and L1ss states are
> + * fully restored.
> + * (enabling ASPM is part of pci_restore_pcie_state)
If we're enabling L1.2 before LTR has been configured, that's
definitely a bug. But I don't see how that happens. The current code
looks like:
pci_restore_state
pci_restore_ltr_state
pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++)
pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++)
pci_restore_aspm_l1ss_state
pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++)
pci_restore_pcie_state
pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++])
We *do* restore PCI_L1SS_CTL1, which contains "ASPM L1.2 Enable",
before we restore PCI_EXP_LNKCTL, which contains "ASPM Control", but I
don't think "ASPM L1.2 Enable" by itself should be enough to allow
hardware to enter L1.2.
Reading PCIe r5.0, sec 5.5.1, it seems like hardware should ignore the
PCI_L1SS_CTL1 bits unless ASPM L1 entry is enabled in PCI_EXP_LNKCTL.
What am I missing?
> + */
> + pcie_config_aspm_dev(dev, 0);
> +
> /*
> * Restore max latencies (in the LTR capability) before enabling
> * LTR itself (in the PCIe capability).
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e9ea5dfaa3e0..f774bd6d2555 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -564,6 +564,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> +void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val);
> void pci_save_aspm_l1ss_state(struct pci_dev *dev);
> void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
> #else
> @@ -571,6 +572,7 @@ static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
> static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> +static inline void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) { }
> static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
> static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
> #endif
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a08e7d6dc248..45535b4e1595 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -778,7 +778,7 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
> pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
> }
>
> -static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> +void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> {
> pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
> PCI_EXP_LNKCTL_ASPMC, val);
> --
> 2.30.0.284.gd98b1dd5eaa7-goog
>
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.
> Signed-off-by: Victor Ding <[email protected]>
> ---
>
> 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
>
Hi Bjorn,
On Wed, Jan 13, 2021 at 9:32 AM Bjorn Helgaas <[email protected]> wrote:
>
> Hi Victor,
>
> Thanks for the patch. Improving suspend/resume performance is always
> a good thing!
>
> On Tue, Jan 12, 2021 at 04:02:04AM +0000, Victor Ding wrote:
> > Right after powering up, the device may have ASPM enabled; however,
> > its LTR and/or L1ss controls may not be in the desired states; hence,
> > the device may enter L1.2 undesirably and cause resume performance
> > penalty. This is especially problematic if ASPM related control
> > registers are modified before a suspension.
>
> s/L1ss/L1SS/ (several occurrences) to match existing usage.
>
I'll update it in the next version.
>
> > Therefore, ASPM should disabled until its LTR and L1ss states are
> > fully restored.
> >
> > Signed-off-by: Victor Ding <[email protected]>
> > ---
> >
> > drivers/pci/pci.c | 11 +++++++++++
> > drivers/pci/pci.h | 2 ++
> > drivers/pci/pcie/aspm.c | 2 +-
> > 3 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index eb323af34f1e..428de433f2e6 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1660,6 +1660,17 @@ void pci_restore_state(struct pci_dev *dev)
> > if (!dev->state_saved)
> > return;
> >
> > + /*
> > + * Right after powering up, the device may have ASPM enabled;
>
> I think this could be stated more precisely. "Right after powering
> up," ASPM should be *disabled* because ASPM Control in the Link
> Control register powers up as zero (disabled).
>
Good suggestion; I'll reword in the next version.
However, ASPM should be *disabled* for the opposite reason - ASPM Control
in the Link Control register *may* power as non-zero (enabled).
(More answered in the next section)
>
> > + * however, its LTR and/or L1ss controls may not be in the desired
> > + * states; as a result, the device may enter L1.2 undesirably and
> > + * cause resume performance penalty.
> > + * Therefore, ASPM is disabled until its LTR and L1ss states are
> > + * fully restored.
> > + * (enabling ASPM is part of pci_restore_pcie_state)
>
> If we're enabling L1.2 before LTR has been configured, that's
> definitely a bug. But I don't see how that happens. The current code
> looks like:
>
> pci_restore_state
> pci_restore_ltr_state
> pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++)
> pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++)
> pci_restore_aspm_l1ss_state
> pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++)
> pci_restore_pcie_state
> pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++])
>
> We *do* restore PCI_L1SS_CTL1, which contains "ASPM L1.2 Enable",
> before we restore PCI_EXP_LNKCTL, which contains "ASPM Control", but I
> don't think "ASPM L1.2 Enable" by itself should be enough to allow
> hardware to enter L1.2.
>
> Reading PCIe r5.0, sec 5.5.1, it seems like hardware should ignore the
> PCI_L1SS_CTL1 bits unless ASPM L1 entry is enabled in PCI_EXP_LNKCTL.
>
> What am I missing?
>
Correct; however, PCI_EXP_LNKCTL may power up as non-zero, i.e. has ASPM
Control enabled. BIOS may set PCI_EXP_LNKCTL (and PCI_L1SS_CTL1) before
Kernel takes control. When BIOS does so, there is a brief period
between powering
up and Kernel restoring LTR state that L1.2 is enabled but LTR isn't configured.
>
> > + */
> > + pcie_config_aspm_dev(dev, 0);
> > +
> > /*
> > * Restore max latencies (in the LTR capability) before enabling
> > * LTR itself (in the PCIe capability).
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index e9ea5dfaa3e0..f774bd6d2555 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -564,6 +564,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
> > void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> > void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> > void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> > +void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val);
> > void pci_save_aspm_l1ss_state(struct pci_dev *dev);
> > void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
> > #else
> > @@ -571,6 +572,7 @@ static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> > static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> > static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
> > static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> > +static inline void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) { }
> > static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
> > static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
> > #endif
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a08e7d6dc248..45535b4e1595 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -778,7 +778,7 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
> > pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
> > }
> >
> > -static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> > +void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> > {
> > pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
> > PCI_EXP_LNKCTL_ASPMC, val);
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >
On Wed, Jan 13, 2021 at 9:38 AM Bjorn Helgaas <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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
> >
[+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 <[email protected]> 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.
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.
> > > Signed-off-by: Victor Ding <[email protected]>
> > > ---
> > >
> > > 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
> > >
On Wed, Jan 13, 2021 at 01:16:05PM +1100, Victor Ding wrote:
> On Wed, Jan 13, 2021 at 9:32 AM Bjorn Helgaas <[email protected]> wrote:
> > On Tue, Jan 12, 2021 at 04:02:04AM +0000, Victor Ding wrote:
> > > Right after powering up, the device may have ASPM enabled; however,
> > > its LTR and/or L1ss controls may not be in the desired states; hence,
> > > the device may enter L1.2 undesirably and cause resume performance
> > > penalty. This is especially problematic if ASPM related control
> > > registers are modified before a suspension.
> >
> > s/L1ss/L1SS/ (several occurrences) to match existing usage.
> >
> I'll update it in the next version.
> >
> > > Therefore, ASPM should disabled until its LTR and L1ss states are
> > > fully restored.
> > >
> > > Signed-off-by: Victor Ding <[email protected]>
> > > ---
> > >
> > > drivers/pci/pci.c | 11 +++++++++++
> > > drivers/pci/pci.h | 2 ++
> > > drivers/pci/pcie/aspm.c | 2 +-
> > > 3 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index eb323af34f1e..428de433f2e6 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -1660,6 +1660,17 @@ void pci_restore_state(struct pci_dev *dev)
> > > if (!dev->state_saved)
> > > return;
> > >
> > > + /*
> > > + * Right after powering up, the device may have ASPM enabled;
> >
> > I think this could be stated more precisely. "Right after powering
> > up," ASPM should be *disabled* because ASPM Control in the Link
> > Control register powers up as zero (disabled).
> >
> Good suggestion; I'll reword in the next version.
> However, ASPM should be *disabled* for the opposite reason - ASPM Control
> in the Link Control register *may* power as non-zero (enabled).
> (More answered in the next section)
> >
> > > + * however, its LTR and/or L1ss controls may not be in the desired
> > > + * states; as a result, the device may enter L1.2 undesirably and
> > > + * cause resume performance penalty.
> > > + * Therefore, ASPM is disabled until its LTR and L1ss states are
> > > + * fully restored.
> > > + * (enabling ASPM is part of pci_restore_pcie_state)
> >
> > If we're enabling L1.2 before LTR has been configured, that's
> > definitely a bug. But I don't see how that happens. The current code
> > looks like:
> >
> > pci_restore_state
> > pci_restore_ltr_state
> > pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++)
> > pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++)
> > pci_restore_aspm_l1ss_state
> > pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++)
> > pci_restore_pcie_state
> > pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++])
> >
> > We *do* restore PCI_L1SS_CTL1, which contains "ASPM L1.2 Enable",
> > before we restore PCI_EXP_LNKCTL, which contains "ASPM Control", but I
> > don't think "ASPM L1.2 Enable" by itself should be enough to allow
> > hardware to enter L1.2.
> >
> > Reading PCIe r5.0, sec 5.5.1, it seems like hardware should ignore the
> > PCI_L1SS_CTL1 bits unless ASPM L1 entry is enabled in PCI_EXP_LNKCTL.
> >
> > What am I missing?
> >
> Correct; however, PCI_EXP_LNKCTL may power up as non-zero, i.e. has
> ASPM Control enabled. BIOS may set PCI_EXP_LNKCTL (and
> PCI_L1SS_CTL1) before Kernel takes control. When BIOS does so, there
> is a brief period between powering up and Kernel restoring LTR state
> that L1.2 is enabled but LTR isn't configured.
IIUC you're saying that ASPM Control powers up as zero, but BIOS
enables ASPM before transferring control to the OS. That makes sense,
but I wouldn't describe that as "the device powering up with ASPM
enabled."
If BIOS enables L1SS (specifically, if it enables L1.2) when LTR
hasn't been configured, that sounds like a BIOS defect.
On Thu, Jan 14, 2021 at 8:48 AM Bjorn Helgaas <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > >
> > > > 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
> > > >
On Thu, Jan 14, 2021 at 7:54 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, Jan 13, 2021 at 01:16:05PM +1100, Victor Ding wrote:
> > On Wed, Jan 13, 2021 at 9:32 AM Bjorn Helgaas <[email protected]> wrote:
> > > On Tue, Jan 12, 2021 at 04:02:04AM +0000, Victor Ding wrote:
> > > > Right after powering up, the device may have ASPM enabled; however,
> > > > its LTR and/or L1ss controls may not be in the desired states; hence,
> > > > the device may enter L1.2 undesirably and cause resume performance
> > > > penalty. This is especially problematic if ASPM related control
> > > > registers are modified before a suspension.
> > >
> > > s/L1ss/L1SS/ (several occurrences) to match existing usage.
> > >
> > I'll update it in the next version.
> > >
> > > > Therefore, ASPM should disabled until its LTR and L1ss states are
> > > > fully restored.
> > > >
> > > > Signed-off-by: Victor Ding <[email protected]>
> > > > ---
> > > >
> > > > drivers/pci/pci.c | 11 +++++++++++
> > > > drivers/pci/pci.h | 2 ++
> > > > drivers/pci/pcie/aspm.c | 2 +-
> > > > 3 files changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index eb323af34f1e..428de433f2e6 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -1660,6 +1660,17 @@ void pci_restore_state(struct pci_dev *dev)
> > > > if (!dev->state_saved)
> > > > return;
> > > >
> > > > + /*
> > > > + * Right after powering up, the device may have ASPM enabled;
> > >
> > > I think this could be stated more precisely. "Right after powering
> > > up," ASPM should be *disabled* because ASPM Control in the Link
> > > Control register powers up as zero (disabled).
> > >
> > Good suggestion; I'll reword in the next version.
> > However, ASPM should be *disabled* for the opposite reason - ASPM Control
> > in the Link Control register *may* power as non-zero (enabled).
> > (More answered in the next section)
> > >
> > > > + * however, its LTR and/or L1ss controls may not be in the desired
> > > > + * states; as a result, the device may enter L1.2 undesirably and
> > > > + * cause resume performance penalty.
> > > > + * Therefore, ASPM is disabled until its LTR and L1ss states are
> > > > + * fully restored.
> > > > + * (enabling ASPM is part of pci_restore_pcie_state)
> > >
> > > If we're enabling L1.2 before LTR has been configured, that's
> > > definitely a bug. But I don't see how that happens. The current code
> > > looks like:
> > >
> > > pci_restore_state
> > > pci_restore_ltr_state
> > > pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++)
> > > pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++)
> > > pci_restore_aspm_l1ss_state
> > > pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++)
> > > pci_restore_pcie_state
> > > pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++])
> > >
> > > We *do* restore PCI_L1SS_CTL1, which contains "ASPM L1.2 Enable",
> > > before we restore PCI_EXP_LNKCTL, which contains "ASPM Control", but I
> > > don't think "ASPM L1.2 Enable" by itself should be enough to allow
> > > hardware to enter L1.2.
> > >
> > > Reading PCIe r5.0, sec 5.5.1, it seems like hardware should ignore the
> > > PCI_L1SS_CTL1 bits unless ASPM L1 entry is enabled in PCI_EXP_LNKCTL.
> > >
> > > What am I missing?
> > >
> > Correct; however, PCI_EXP_LNKCTL may power up as non-zero, i.e. has
> > ASPM Control enabled. BIOS may set PCI_EXP_LNKCTL (and
> > PCI_L1SS_CTL1) before Kernel takes control. When BIOS does so, there
> > is a brief period between powering up and Kernel restoring LTR state
> > that L1.2 is enabled but LTR isn't configured.
>
> IIUC you're saying that ASPM Control powers up as zero, but BIOS
> enables ASPM before transferring control to the OS. That makes sense,
> but I wouldn't describe that as "the device powering up with ASPM
> enabled."
>
Correct.
>
> If BIOS enables L1SS (specifically, if it enables L1.2) when LTR
> hasn't been configured, that sounds like a BIOS defect.
>
Very good point. I'll withdraw this patch then.