| Message ID | 20210112040146.2.Ic902bbd9f04e2d82ac578411e7fafc77b6c750e2@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Disable ASPM on GL9750 during a suspension | expand |
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 <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 >
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 > >
[+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@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? 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 <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 > > >
On Thu, Jan 14, 2021 at 8:48 AM Bjorn Helgaas <helgaas@kernel.org> 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@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? > > 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@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 > > > >
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);
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 <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(-)