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 use S0ix as the suspend mechanism: 1. Userspace initiates s2idle suspend (e.g. via writing to /sys/power/state) 2. This switches the runtime_pm device state to active, which disables LPM negotiation, then calls the "regular" suspend callback 3. With LPM negotiation disabled, the bus cannot enter low-power state 4. 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 # on gladios device # on 15590.0.0 with v5.10 and upstream (v6.4) kernels
---
Changes in v2: - improved symmetry and error path in s2idle suspend callback (internal review)
drivers/mmc/host/sdhci-pci-gli.c | 102 +++++++++++++++++++------------ 1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 1792665c9494a..19f577cc8bceb 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,68 @@ 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 +1139,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,
Hi
Looks OK - a few minor comments below
On 23/08/23 20:41, Sven van Ashbrook 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
If possible, can you give example of which SoCs / products
which use S0ix as the suspend mechanism:
- 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 # on gladios device # on 15590.0.0 with v5.10 and upstream (v6.4) kernels
3 extraneous lines here - please remove
Changes in v2:
- improved symmetry and error path in s2idle suspend callback (internal review)
drivers/mmc/host/sdhci-pci-gli.c | 102 +++++++++++++++++++------------ 1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 1792665c9494a..19f577cc8bceb 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,68 @@ 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.*/
I would prefer the comment style:
/* * Blah, blah ... * Blah, blah, blah. */
- 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-
Ditto re comment style
* 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);
Couldn't this be at the end of the function, save an error path
- 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 +1139,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,
On 24/08/23 14:46, Adrian Hunter wrote:
Hi
Looks OK - a few minor comments below
On 23/08/23 20:41, Sven van Ashbrook 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
If possible, can you give example of which SoCs / products
which use S0ix as the suspend mechanism:
- 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 # on gladios device # on 15590.0.0 with v5.10 and upstream (v6.4) kernels
3 extraneous lines here - please remove
Changes in v2:
- improved symmetry and error path in s2idle suspend callback (internal review)
drivers/mmc/host/sdhci-pci-gli.c | 102 +++++++++++++++++++------------ 1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 1792665c9494a..19f577cc8bceb 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,68 @@ 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.*/I would prefer the comment style:
/* * Blah, blah ... * Blah, blah, blah. */
- 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-
Ditto re comment style
* 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);
Couldn't this be at the end of the function, save an error path
I think the discussion was a little uncertain over moving this, so maybe leave it alone.
However please consider my other comments.
- 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 +1139,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,
Hi Adrian,
On Thu, Aug 31, 2023 at 9:02 AM Adrian Hunter adrian.hunter@intel.com wrote:
I think the discussion was a little uncertain over moving this, so maybe leave it alone.
Agree, will do.
However please consider my other comments.
Of course !
linux-stable-mirror@lists.linaro.org
-
Adrian Hunter -
Sven van Ashbrook