On 1/09/23 09:33, Lai Jason wrote:
Hi,
On Fri, Sep 1, 2023 at 12:01 AM Sven van Ashbrook svenva@chromium.org wrote:
To improve the r/w performance of GL9763E, the current driver inhibits LPM negotiation while the device is active.
This prevents a large number of SoCs from suspending, notably x86 systems which commonly use S0ix as the suspend mechanism - for example, Intel Alder Lake and Raptor Lake processors.
Failure description:
- Userspace initiates s2idle suspend (e.g. via writing to /sys/power/state)
- This switches the runtime_pm device state to active, which disables LPM negotiation, then calls the "regular" suspend callback
- With LPM negotiation disabled, the bus cannot enter low-power state
- On a large number of SoCs, if the bus not in a low-power state, S0ix cannot be entered, which in turn prevents the SoC from entering suspend.
Fix by re-enabling LPM negotiation in the device's suspend callback.
Suggested-by: Stanislaw Kardach skardach@google.com Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E") Cc: stable@vger.kernel.org Signed-off-by: Sven van Ashbrook svenva@chromium.org
LGTM. But I prefer Ben's opinion "I suppose cqhci_suspend() may need to be done first safely, then gl9763e_set_low_power_negotiation(slot, true)." because it can avoid GL9763E entering L1 while cmdq engine processes I/O.
The block device is a child of the host controller so must already be suspended at this point. So there is no I/O in process.
cqhci_suspend() just turns the cmdq engine off.
So I don't think a change is needed, but please correct me if I am wrong.
Best regards, Jason Lai
Changes in v3:
- applied maintainer feedback from https://lore.kernel.org/lkml/CACT4zj-BaX4tHji8B8gS5jiKkd-2BcwfzHM4fS-OUn0f8D...
Changes in v2:
- improved symmetry and error path in s2idle suspend callback (internal review)
drivers/mmc/host/sdhci-pci-gli.c | 104 ++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 38 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 1792665c9494a..a4ccb6c3e27a6 100644 --- a/drivers/mmc/host/sdhci-pci-gli.c +++ b/drivers/mmc/host/sdhci-pci-gli.c @@ -745,42 +745,6 @@ static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg) return value; }
-#ifdef CONFIG_PM_SLEEP -static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip) -{
struct sdhci_pci_slot *slot = chip->slots[0];pci_free_irq_vectors(slot->chip->pdev);gli_pcie_enable_msi(slot);return sdhci_pci_resume_host(chip);-}
-static int sdhci_cqhci_gli_resume(struct sdhci_pci_chip *chip) -{
struct sdhci_pci_slot *slot = chip->slots[0];int ret;ret = sdhci_pci_gli_resume(chip);if (ret)return ret;return cqhci_resume(slot->host->mmc);-}
-static int sdhci_cqhci_gli_suspend(struct sdhci_pci_chip *chip) -{
struct sdhci_pci_slot *slot = chip->slots[0];int ret;ret = cqhci_suspend(slot->host->mmc);if (ret)return ret;return sdhci_suspend_host(slot->host);-} -#endif
static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc, struct mmc_ios *ios) { @@ -1029,6 +993,70 @@ static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip) } #endif
+#ifdef CONFIG_PM_SLEEP +static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip) +{
struct sdhci_pci_slot *slot = chip->slots[0];pci_free_irq_vectors(slot->chip->pdev);gli_pcie_enable_msi(slot);return sdhci_pci_resume_host(chip);+}
+static int gl9763e_resume(struct sdhci_pci_chip *chip) +{
struct sdhci_pci_slot *slot = chip->slots[0];int ret;ret = sdhci_pci_gli_resume(chip);if (ret)return ret;ret = cqhci_resume(slot->host->mmc);if (ret)return ret;/** Disable LPM negotiation to bring device back in sync* with its runtime_pm state.*/gl9763e_set_low_power_negotiation(slot, false);return 0;+}
+static int gl9763e_suspend(struct sdhci_pci_chip *chip) +{
struct sdhci_pci_slot *slot = chip->slots[0];int ret;/** Certain SoCs can suspend only with the bus in low-* power state, notably x86 SoCs when using S0ix.* Re-enable LPM negotiation to allow entering L1 state* and entering system suspend.*/gl9763e_set_low_power_negotiation(slot, true);ret = cqhci_suspend(slot->host->mmc);if (ret)goto err_suspend;ret = sdhci_suspend_host(slot->host);if (ret)goto err_suspend_host;return 0;+err_suspend_host:
cqhci_resume(slot->host->mmc);+err_suspend:
gl9763e_set_low_power_negotiation(slot, false);return ret;+} +#endif
static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot) { struct pci_dev *pdev = slot->chip->pdev; @@ -1113,8 +1141,8 @@ const struct sdhci_pci_fixes sdhci_gl9763e = { .probe_slot = gli_probe_slot_gl9763e, .ops = &sdhci_gl9763e_ops, #ifdef CONFIG_PM_SLEEP
.resume = sdhci_cqhci_gli_resume,.suspend = sdhci_cqhci_gli_suspend,
.resume = gl9763e_resume,.suspend = gl9763e_suspend,#endif #ifdef CONFIG_PM .runtime_suspend = gl9763e_runtime_suspend, -- 2.42.0.283.g2d96d420d3-goog