1. Patchew
  2. linux
  3. View series

[PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend

Sven van Ashbrook posted 1 patch 2 years, 3 months ago
There is a newer version of this series
drivers/mmc/host/sdhci-pci-gli.c | 102 +++++++++++++++++++------------
1 file changed, 64 insertions(+), 38 deletions(-)
[PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend
Posted by Sven van Ashbrook 2 years, 3 months ago 
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,
-- 
2.42.0.rc1.204.g551eb34607-goog
Re: [PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend
Posted by Adrian Hunter 2 years, 3 months ago 
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:
> 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
> 

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,
Re: [PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend
Posted by Adrian Hunter 2 years, 3 months ago 
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:
>> 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
>>
> 
> 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,
>
Re: [PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend
Posted by Sven van Ashbrook 2 years, 3 months ago 
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 !

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.