* [PATCH 0/2] Disable ASPM on GL9750 during a suspension @ 2021-01-12 4:02 Victor Ding 2021-01-12 4:02 ` [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored Victor Ding 2021-01-12 4:02 ` [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension Victor Ding 0 siblings, 2 replies; 11+ messages in thread From: Victor Ding @ 2021-01-12 4:02 UTC (permalink / raw) To: Ulf Hansson, Adrian Hunter Cc: Ben Chuang, Bjorn Helgaas, linux-kernel, linux-pci, linux-mmc, Victor Ding, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored 2021-01-12 4:02 [PATCH 0/2] Disable ASPM on GL9750 during a suspension Victor Ding @ 2021-01-12 4:02 ` Victor Ding 2021-01-12 22:32 ` Bjorn Helgaas 2021-01-12 4:02 ` [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension Victor Ding 1 sibling, 1 reply; 11+ messages in thread From: Victor Ding @ 2021-01-12 4:02 UTC (permalink / raw) To: Ulf Hansson, Adrian Hunter Cc: Ben Chuang, Bjorn Helgaas, linux-kernel, linux-pci, linux-mmc, Victor Ding, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang 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 <victording@google.com> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored 2021-01-12 4:02 ` [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored Victor Ding @ 2021-01-12 22:32 ` Bjorn Helgaas 2021-01-13 2:16 ` Victor Ding 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2021-01-12 22:32 UTC (permalink / raw) To: Victor Ding Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, linux-kernel, linux-pci, linux-mmc, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang 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 <victording@google.com> > --- > > 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored 2021-01-12 22:32 ` Bjorn Helgaas @ 2021-01-13 2:16 ` Victor Ding 2021-01-13 20:54 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Victor Ding @ 2021-01-13 2:16 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang Hi Bjorn, On Wed, Jan 13, 2021 at 9:32 AM Bjorn Helgaas <helgaas@kernel.org> 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 <victording@google.com> > > --- > > > > 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored 2021-01-13 2:16 ` Victor Ding @ 2021-01-13 20:54 ` Bjorn Helgaas 2021-01-14 9:13 ` Victor Ding 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2021-01-13 20:54 UTC (permalink / raw) To: Victor Ding Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang On Wed, Jan 13, 2021 at 01:16:05PM +1100, Victor Ding wrote: > On Wed, Jan 13, 2021 at 9:32 AM Bjorn Helgaas <helgaas@kernel.org> 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 <victording@google.com> > > > --- > > > > > > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored 2021-01-13 20:54 ` Bjorn Helgaas @ 2021-01-14 9:13 ` Victor Ding 0 siblings, 0 replies; 11+ messages in thread From: Victor Ding @ 2021-01-14 9:13 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc, Alex Levin, Bjorn Helgaas, Kai-Heng Feng, Saheed O. Bolarinwa, Sean Paul, Sukumar Ghorai, Yicong Yang On Thu, Jan 14, 2021 at 7:54 AM Bjorn Helgaas <helgaas@kernel.org> 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 <helgaas@kernel.org> 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 <victording@google.com> > > > > --- > > > > > > > > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension 2021-01-12 4:02 [PATCH 0/2] Disable ASPM on GL9750 during a suspension Victor Ding 2021-01-12 4:02 ` [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored Victor Ding @ 2021-01-12 4:02 ` Victor Ding 2021-01-12 22:38 ` Bjorn Helgaas 1 sibling, 1 reply; 11+ messages in thread From: Victor Ding @ 2021-01-12 4:02 UTC (permalink / raw) To: Ulf Hansson, Adrian Hunter Cc: Ben Chuang, Bjorn Helgaas, linux-kernel, linux-pci, linux-mmc, Victor Ding 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(-) 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension 2021-01-12 4:02 ` [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension Victor Ding @ 2021-01-12 22:38 ` Bjorn Helgaas 2021-01-13 2:16 ` Victor Ding 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2021-01-12 22:38 UTC (permalink / raw) To: Victor Ding Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, linux-kernel, linux-pci, linux-mmc 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension 2021-01-12 22:38 ` Bjorn Helgaas @ 2021-01-13 2:16 ` Victor Ding 2021-01-13 21:48 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Victor Ding @ 2021-01-13 2:16 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension 2021-01-13 2:16 ` Victor Ding @ 2021-01-13 21:48 ` Bjorn Helgaas 2021-01-14 9:13 ` Victor Ding 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2021-01-13 21:48 UTC (permalink / raw) To: Victor Ding Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc, Rafael J. Wysocki [+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 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension 2021-01-13 21:48 ` Bjorn Helgaas @ 2021-01-14 9:13 ` Victor Ding 0 siblings, 0 replies; 11+ messages in thread From: Victor Ding @ 2021-01-14 9:13 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ulf Hansson, Adrian Hunter, Ben Chuang, LKML, linux-pci, linux-mmc, Rafael J. Wysocki 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 > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-01-14 9:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-12 4:02 [PATCH 0/2] Disable ASPM on GL9750 during a suspension Victor Ding 2021-01-12 4:02 ` [PATCH 1/2] PCI/ASPM: Disable ASPM until its LTR and L1ss state is restored Victor Ding 2021-01-12 22:32 ` Bjorn Helgaas 2021-01-13 2:16 ` Victor Ding 2021-01-13 20:54 ` Bjorn Helgaas 2021-01-14 9:13 ` Victor Ding 2021-01-12 4:02 ` [PATCH 2/2] mmc: sdhci-pci-gli: Disable ASPM during a suspension Victor Ding 2021-01-12 22:38 ` Bjorn Helgaas 2021-01-13 2:16 ` Victor Ding 2021-01-13 21:48 ` Bjorn Helgaas 2021-01-14 9:13 ` Victor Ding
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.