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

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

[Sven van Ashbrook]

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

[Adrian Hunter]

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

[Adrian Hunter]

On 24/08/23 14:50, Stanisław Kardach wrote:
> Hi Adrian,
> 
> Thanks for reviewing our patches.
> 
> On Thu, Aug 24, 2023 at 1:47 PM Adrian Hunter <adrian.hunter@intel.com <mailto:adrian.hunter@intel.com>> 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 <mailto:skardach@google.com>>
>     > Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
>     > Cc: stable@vger.kernel.org <mailto:stable@vger.kernel.org>
>     > Signed-off-by: Sven van Ashbrook <svenva@chromium.org <mailto: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
> 
> Please correct me if I'm wrong but writing to device config
> space could trigger a side effect, so it's probably better to
> do it before calling functions suspending the device?

sdhci doesn't know anything about the bus.  It is independent
of PCI, so I can't see how it would make any difference.
One of the people cc'ed might know more.  Jason Lai (cc'ed)
added it for runtime PM.

> 
> 
>     > +
>     > +     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,
> 
> 
> 
> -- 
> Best Regards,
> Stanisław Kardach

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

[Ben Chuang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 8731 bytes --]

Hi,

On 24/08/23 20:18, Adrian Hunter wrote:
> On 24/08/23 14:50, Stanisław Kardach wrote:
> > Hi Adrian,
> > 
> > Thanks for reviewing our patches.
> > 
> > On Thu, Aug 24, 2023 at 1:47 PM Adrian Hunter <adrian.hunter@intel.com <mailto:adrian.hunter@intel.com>> 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 <mailto:skardach@google.com>>
> >     > Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> >     > Cc: stable@vger.kernel.org <mailto:stable@vger.kernel.org>
> >     > Signed-off-by: Sven van Ashbrook <svenva@chromium.org <mailto: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);
> >     > +}

sdhci_pci_gli_resume() is the same as before. Is there any reason to move it here?

> >     > +
> >     > +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);

There is a situation for your reference.
If `allow_runtime_pm' is set to false and the system resumes from suspend, GL9763E 
LPM negotiation will be always disabled on S0. GL9763E will stay L0 and never 
enter L1 because GL9763E LPM negotiation is disabled.

This patch enables allow_runtime_pm. The simple flow is
gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a)
(a) -+--> idle -->  gl9763e_runtime_suspend() -> LPM enabled
     |
     +--> no idle -> gl9763e_runtime_resume() -> LPM disabled

This patch disables allow_runtime_pm. The simple flow is
gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled (no runtime_pm)

Although that may not be the case with the current configuration, it's only a
possibility.

> >     > +
> >     > +     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
> > 
> > Please correct me if I'm wrong but writing to device config
> > space could trigger a side effect, so it's probably better to
> > do it before calling functions suspending the device?
> 
> sdhci doesn't know anything about the bus.  It is independent
> of PCI, so I can't see how it would make any difference.
> One of the people cc'ed might know more.  Jason Lai (cc'ed)
> added it for runtime PM.
>

As far as I know, when disabling LPM negotiation, the GL9763E will stop entering
L1. It doesn't other side effect. Does Jason.Lai and Victor.Shih have any comments
or suggestions?

Best regards,
Ben Chuang

> > 
> > 
> >     > +
> >     > +     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,
> > 
> > 
> > 
> > -- 
> > Best Regards,
> > Stanisław Kardach
> 
> 

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

[Stanisław Kardach]

Hi Ben,

Thanks for reviewing our patch.

On Mon, Aug 28, 2023 at 12:03 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> Hi,
>
> On 24/08/23 20:18, Adrian Hunter wrote:
> > On 24/08/23 14:50, Stanisław Kardach wrote:
> > > Hi Adrian,
> > >
> > > Thanks for reviewing our patches.
> > >
> > > On Thu, Aug 24, 2023 at 1:47 PM Adrian Hunter <adrian.hunter@intel.com <mailto:adrian.hunter@intel.com>> 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 <mailto:skardach@google.com>>
> > >     > Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> > >     > Cc: stable@vger.kernel.org <mailto:stable@vger.kernel.org>
> > >     > Signed-off-by: Sven van Ashbrook <svenva@chromium.org <mailto: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);
> > >     > +}
>
> sdhci_pci_gli_resume() is the same as before. Is there any reason to move it here?
To avoid having multiple #ifdef CONFIG_PM_SLEEP blocks. We can leave
it, where it
was if you prefer.
>
> > >     > +
> > >     > +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);
>
> There is a situation for your reference.
> If `allow_runtime_pm' is set to false and the system resumes from suspend, GL9763E
> LPM negotiation will be always disabled on S0. GL9763E will stay L0 and never
> enter L1 because GL9763E LPM negotiation is disabled.
>
> This patch enables allow_runtime_pm. The simple flow is
> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a)
> (a) -+--> idle -->  gl9763e_runtime_suspend() -> LPM enabled
>      |
>      +--> no idle -> gl9763e_runtime_resume() -> LPM disabled
Is the lower branch of this sequence possible? I mean please correct
me if I'm wrong
but after the s2idle resume, devices are considered runtime active unless deemed
otherwise, so gl_9763e_resume() -> fl9763e_runtime_resume() should not
happen, right?
>
> This patch disables allow_runtime_pm. The simple flow is
> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled (no runtime_pm)
>
> Although that may not be the case with the current configuration, it's only a
> possibility.
Actually last year there was a patch to improve R/W performance of GL9763E:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9e5b33934cec24b8c024add5c5d65d2f93ade05
If I'm reading the code right we're not modifying allow_runtime_pm,
rather we're disabling
the L1 negotiation in the same manner as the abovementioned patch does
in the runtime
PM flow. So in a way we're restoring whatever setting was there except
a situation where
the runtime PM was disabled from the start on the device and we do not
restore the
original state of the LPM negotiation fields. Not sure if such case is
really possible.
>
> > >     > +
> > >     > +     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
> > >
> > > Please correct me if I'm wrong but writing to device config
> > > space could trigger a side effect, so it's probably better to
> > > do it before calling functions suspending the device?
> >
> > sdhci doesn't know anything about the bus.  It is independent
> > of PCI, so I can't see how it would make any difference.
> > One of the people cc'ed might know more.  Jason Lai (cc'ed)
> > added it for runtime PM.
> >
>
> As far as I know, when disabling LPM negotiation, the GL9763E will stop entering
> L1. It doesn't other side effect. Does Jason.Lai and Victor.Shih have any comments
> or suggestions?
The reason we've put the LPM negotiation handling at the start of
suspend and finish
of resume was mostly for semantical reasons - first do device logic,
then call base
framework/module logic which might expect all the device-specific steps to be
performed already. Maybe it does not make any difference in the real world for
SDHCI controllers but it just seemed to look better. Also suspend and resume
callbacks already have such ordering when it comes to cqhci_* and sdhci_*
functions.
>
> Best regards,
> Ben Chuang
>
> > >
> > >
> > >     > +
> > >     > +     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,
> > >
> > >
> > >
> > > --
> > > Best Regards,
> > > Stanisław Kardach
> >
> >



-- 
Best Regards,
Stanisław Kardach

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

[Sven van Ashbrook]

Hi Ben, thank you for reviewing this patch. See below.

On Mon, Aug 28, 2023 at 6:03 AM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> There is a situation for your reference.
> If `allow_runtime_pm' is set to false and the system resumes from suspend, GL9763E
> LPM negotiation will be always disabled on S0. GL9763E will stay L0 and never
> enter L1 because GL9763E LPM negotiation is disabled.
>
> This patch enables allow_runtime_pm. The simple flow is
> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a)
> (a) -+--> idle -->  gl9763e_runtime_suspend() -> LPM enabled
>      |
>      +--> no idle -> gl9763e_runtime_resume() -> LPM disabled
>
> This patch disables allow_runtime_pm. The simple flow is
> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled (no runtime_pm)
>
> Although that may not be the case with the current configuration, it's only a
> possibility.
>

Thanks so much for bringing this up. We have discussed internally and
as far as we know, the current patch will work correctly in all cases.
Could you verify our argument please?

The following assumptions are key:

1. If CONFIG_PM is set, the runtime_pm framework is always present, i.e. there
cannot exist a kernel which has PM but lacks runtime_pm.
2. The pm_runtime framework always makes sure the device is runtime
active before
calling XXX_suspend, waking it up if need be. So when XXX_suspend gets called,
the device is always runtime active.
3. if CONFIG_PM is set, runtime_pm can only be disabled via
echo on > /sys/devices/.../power/control, and then the runtime_pm framework
always keeps the device in runtime active. In such case LPM negotiation is
always disabled.

Using these assumptions, we get:

Runtime_pm allowed:
—------------------
gl9763e_runtime_resume() -> LPM disabled -> gl9763e_suspend() -> LPM enabled
-> gl9763e_resume() -> LPM disabled -> (a)
(a) -+--> idle --> gl9763e_runtime_suspend() -> LPM enabled
|
+--> no idle -> nothing - already runtime active -> LPM disabled

Runtime_pm not allowed:
—----------------------
gl9763e_runtime_resume() always called -> LPM always disabled
gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled

In all above cases the LPM negotiation flag is correct.

> >
> > sdhci doesn't know anything about the bus.  It is independent
> > of PCI, so I can't see how it would make any difference.
> > One of the people cc'ed might know more.  Jason Lai (cc'ed)
> > added it for runtime PM.
> >
>
> As far as I know, when disabling LPM negotiation, the GL9763E will stop entering
> L1. It doesn't other side effect. Does Jason.Lai and Victor.Shih have any comments
> or suggestions?

Sounds like everyone assumes that you can freely change LPM
negotiation on the PCIe
bus after the cqhci_suspend() and sdhci_suspend_host() calls, so we will assume
that too.

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

[Ben Chuang]

Hi Sven and Stanisław,

Sorry for Stanisław, I have seen your reply, but I only reply to this
Regarding the location of the source codes, if the maintainers don't
comment. I'll follow them too. :)

On Tue, Aug 29, 2023 at 6:51 AM Sven van Ashbrook <svenva@chromium.org> wrote:
>
> Hi Ben, thank you for reviewing this patch. See below.
>
> On Mon, Aug 28, 2023 at 6:03 AM Ben Chuang <benchuanggli@gmail.com> wrote:
> >
> > There is a situation for your reference.
> > If `allow_runtime_pm' is set to false and the system resumes from suspend, GL9763E
> > LPM negotiation will be always disabled on S0. GL9763E will stay L0 and never
> > enter L1 because GL9763E LPM negotiation is disabled.
> >
> > This patch enables allow_runtime_pm. The simple flow is
> > gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a)
> > (a) -+--> idle -->  gl9763e_runtime_suspend() -> LPM enabled
> >      |
> >      +--> no idle -> gl9763e_runtime_resume() -> LPM disabled
> >
> > This patch disables allow_runtime_pm. The simple flow is
> > gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled (no runtime_pm)
> >
> > Although that may not be the case with the current configuration, it's only a
> > possibility.
> >
>
> Thanks so much for bringing this up. We have discussed internally and
> as far as we know, the current patch will work correctly in all cases.
> Could you verify our argument please?
>
> The following assumptions are key:
>
> 1. If CONFIG_PM is set, the runtime_pm framework is always present, i.e. there
> cannot exist a kernel which has PM but lacks runtime_pm.
> 2. The pm_runtime framework always makes sure the device is runtime
> active before
> calling XXX_suspend, waking it up if need be. So when XXX_suspend gets called,
> the device is always runtime active.
> 3. if CONFIG_PM is set, runtime_pm can only be disabled via
> echo on > /sys/devices/.../power/control, and then the runtime_pm framework
> always keeps the device in runtime active. In such case LPM negotiation is
> always disabled.
>
> Using these assumptions, we get:
>
> Runtime_pm allowed:
> —------------------
> gl9763e_runtime_resume() -> LPM disabled -> gl9763e_suspend() -> LPM enabled
> -> gl9763e_resume() -> LPM disabled -> (a)
> (a) -+--> idle --> gl9763e_runtime_suspend() -> LPM enabled
> |
> +--> no idle -> nothing - already runtime active -> LPM disabled
>
> Runtime_pm not allowed:
> —----------------------
> gl9763e_runtime_resume() always called -> LPM always disabled
> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
>
> In all above cases the LPM negotiation flag is correct.
>

My concern is that when runtime_pm is false, gl9763e is disabled LPM
negotiation, gl9763e can't enter L1.x and s0ix may fail.
It seems that runtime_pm will always exist and that's ok.

> > >
> > > sdhci doesn't know anything about the bus.  It is independent
> > > of PCI, so I can't see how it would make any difference.
> > > One of the people cc'ed might know more.  Jason Lai (cc'ed)
> > > added it for runtime PM.
> > >
> >
> > As far as I know, when disabling LPM negotiation, the GL9763E will stop entering
> > L1. It doesn't other side effect. Does Jason.Lai and Victor.Shih have any comments
> > or suggestions?
>
> Sounds like everyone assumes that you can freely change LPM
> negotiation on the PCIe
> bus after the cqhci_suspend() and sdhci_suspend_host() calls, so we will assume
> that too.

Ah, I suppose cqhci_suspend() may need to be done first safely, then
gl9763e_set_low_power_negotiation(slot, true).

Best regards,
Ben Chuang

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

[Sven van Ashbrook]

+ Rafael for advice on runtime_pm corner cases.

On Mon, Aug 28, 2023 at 10:48 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>
>
> My concern is that when runtime_pm is false, gl9763e is disabled LPM
> negotiation, gl9763e can't enter L1.x and s0ix may fail.
> It seems that runtime_pm will always exist and that's ok.
>

Thank you. I believe we can address your concern.

- XXX_suspend/XXX_resume (i.e. classic suspend/resume) depends on
  CONFIG_PM_SLEEP. This always selects CONFIG_PM. This always includes
  the runtime_pm framework. So, if XXX_suspend/XXX_resume gets called,
  the runtime_pm framework is always present, but may not be actively
  managing the device.

- "when runtime_pm is false" AFAIK the only way to disable runtime_pm
  when CONFIG_PM is set, is to write "on" to /sys/devices/.../power/control.
  See https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power
  In that case, the runtime_pm framework will activate the device, calling
  XXX_runtime_resume() if necessary. Are there other ways of disabling it?

- if /sys/devices/.../power/control is "on", then:
  gl9763e_runtime_resume() always called -> LPM always disabled
  gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
  In between "classic" XXX_suspend and XXX_resume, LPM will be enabled,
  so the device can enter L1.x and S0ix.

And the LPM negotiation flags look correct.
Does that address your concerns?

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

[Ben Chuang]

Hi,
On Wed, Aug 30, 2023 at 12:35 AM Sven van Ashbrook <svenva@chromium.org> wrote:
>
> + Rafael for advice on runtime_pm corner cases.
>
> On Mon, Aug 28, 2023 at 10:48 PM Ben Chuang <benchuanggli@gmail.com> wrote:
> >
> >
> > My concern is that when runtime_pm is false, gl9763e is disabled LPM
> > negotiation, gl9763e can't enter L1.x and s0ix may fail.
> > It seems that runtime_pm will always exist and that's ok.
> >
>
> Thank you. I believe we can address your concern.
>
> - XXX_suspend/XXX_resume (i.e. classic suspend/resume) depends on
>   CONFIG_PM_SLEEP. This always selects CONFIG_PM. This always includes
>   the runtime_pm framework. So, if XXX_suspend/XXX_resume gets called,
>   the runtime_pm framework is always present, but may not be actively
>   managing the device.
This is ok.

>
> - "when runtime_pm is false" AFAIK the only way to disable runtime_pm
>   when CONFIG_PM is set, is to write "on" to /sys/devices/.../power/control.
>   See https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power
>   In that case, the runtime_pm framework will activate the device, calling
>   XXX_runtime_resume() if necessary. Are there other ways of disabling it?
>
> - if /sys/devices/.../power/control is "on", then:
>   gl9763e_runtime_resume() always called -> LPM always disabled
>   gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
>   In between "classic" XXX_suspend and XXX_resume, LPM will be enabled,
>   so the device can enter L1.x and S0ix.
In this cas, after gl9763e_resume(), it is LPM disabled.
Is there no chance for gl9763e to enter L1.x again when the system is idle?

>
> And the LPM negotiation flags look correct.
> Does that address your concerns?

Best regards,
Ben Chuang

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

[Stanisław Kardach]

On Wed, Aug 30, 2023 at 4:27 AM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> Hi,
> On Wed, Aug 30, 2023 at 12:35 AM Sven van Ashbrook <svenva@chromium.org> wrote:
> >
> > + Rafael for advice on runtime_pm corner cases.
> >
> > On Mon, Aug 28, 2023 at 10:48 PM Ben Chuang <benchuanggli@gmail.com> wrote:
> > >
> > >
> > > My concern is that when runtime_pm is false, gl9763e is disabled LPM
> > > negotiation, gl9763e can't enter L1.x and s0ix may fail.
> > > It seems that runtime_pm will always exist and that's ok.
> > >
> >
> > Thank you. I believe we can address your concern.
> >
> > - XXX_suspend/XXX_resume (i.e. classic suspend/resume) depends on
> >   CONFIG_PM_SLEEP. This always selects CONFIG_PM. This always includes
> >   the runtime_pm framework. So, if XXX_suspend/XXX_resume gets called,
> >   the runtime_pm framework is always present, but may not be actively
> >   managing the device.
> This is ok.
>
> >
> > - "when runtime_pm is false" AFAIK the only way to disable runtime_pm
> >   when CONFIG_PM is set, is to write "on" to /sys/devices/.../power/control.
> >   See https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power
> >   In that case, the runtime_pm framework will activate the device, calling
> >   XXX_runtime_resume() if necessary. Are there other ways of disabling it?
> >
> > - if /sys/devices/.../power/control is "on", then:
> >   gl9763e_runtime_resume() always called -> LPM always disabled
> >   gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
> >   In between "classic" XXX_suspend and XXX_resume, LPM will be enabled,
> >   so the device can enter L1.x and S0ix.
> In this cas, after gl9763e_resume(), it is LPM disabled.
> Is there no chance for gl9763e to enter L1.x again when the system is idle?
With runtime PM disabled via sysfs, the short answer is not since
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9e5b33934cec24b8c024add5c5d65d2f93ade05.

The longer answer is:
1. System boots up with LPM flags in PCI config space in default value
(might be LPM enabled).
2.1. If runtime PM is disabled before first runtime suspend -
registers will be left in their default state.
2.2. If runtime PM is disabled after first runtime suspend, the device
will be woken up and the gl9763e runtime resume callback will disable
LPM.
>
> >
> > And the LPM negotiation flags look correct.
> > Does that address your concerns?
>
> Best regards,
> Ben Chuang

--
Best Regards,
Stanisław Kardach

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

[Sven van Ashbrook]

On Tue, Aug 29, 2023 at 10:27 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> >
> > - if /sys/devices/.../power/control is "on", then:
> >   <snip>
> >
> In this cas, after gl9763e_resume(), it is LPM disabled.
> Is there no chance for gl9763e to enter L1.x again when the system is idle?
>

AFAIK the only way to disable runtime_pm is to write:
  $ echo on > /sys/devices/.../power/control
where
  $ echo auto > /sys/devices/.../power/control
      means: runtime_pm is actively managing the device, device can be "active"
      or "suspended".
  $ echo on > /sys/devices/.../power/control
      means: runtime_pm is not managing the device, device is "active" only.

In the "auto" case, we know what should happen: LPM negotiation is enabled when
idle, disabled when active.

What should be the LPM negotiation state in the "on" case? We have to
make a choice:
a) LPM negotiation disabled: normal performance, high power consumption, OR
b) LPM negotiation  enabled: low    performance, low  power consumption

If userspace disables our device's runtime_pm by writing "on", it expects the
device to be always-on. It should then expect a higher power consumption.
It should then also expect a performance that is not-worse than the "auto" case.

So my suggestion would be to use (a), which is what this patch does.

Appreciate your thoughts.

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

[Ben Chuang]

Hi Stanisław,

On Wed, Aug 30, 2023 at 3:32 PM Stanisław Kardach <skardach@google.com> wrote:
>
> On Wed, Aug 30, 2023 at 4:27 AM Ben Chuang <benchuanggli@gmail.com> wrote:
> >
> > Hi,
> > On Wed, Aug 30, 2023 at 12:35 AM Sven van Ashbrook <svenva@chromium.org> wrote:
> > >
> > > + Rafael for advice on runtime_pm corner cases.
> > >
> > > On Mon, Aug 28, 2023 at 10:48 PM Ben Chuang <benchuanggli@gmail.com> wrote:
> > > >
> > > >
> > > > My concern is that when runtime_pm is false, gl9763e is disabled LPM
> > > > negotiation, gl9763e can't enter L1.x and s0ix may fail.
> > > > It seems that runtime_pm will always exist and that's ok.
> > > >
> > >
> > > Thank you. I believe we can address your concern.
> > >
> > > - XXX_suspend/XXX_resume (i.e. classic suspend/resume) depends on
> > >   CONFIG_PM_SLEEP. This always selects CONFIG_PM. This always includes
> > >   the runtime_pm framework. So, if XXX_suspend/XXX_resume gets called,
> > >   the runtime_pm framework is always present, but may not be actively
> > >   managing the device.
> > This is ok.
> >
> > >
> > > - "when runtime_pm is false" AFAIK the only way to disable runtime_pm
> > >   when CONFIG_PM is set, is to write "on" to /sys/devices/.../power/control.
> > >   See https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power
> > >   In that case, the runtime_pm framework will activate the device, calling
> > >   XXX_runtime_resume() if necessary. Are there other ways of disabling it?
> > >
> > > - if /sys/devices/.../power/control is "on", then:
> > >   gl9763e_runtime_resume() always called -> LPM always disabled
> > >   gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
> > >   In between "classic" XXX_suspend and XXX_resume, LPM will be enabled,
> > >   so the device can enter L1.x and S0ix.
> > In this cas, after gl9763e_resume(), it is LPM disabled.
> > Is there no chance for gl9763e to enter L1.x again when the system is idle?
> With runtime PM disabled via sysfs, the short answer is not since
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9e5b33934cec24b8c024add5c5d65d2f93ade05.
>
> The longer answer is:
> 1. System boots up with LPM flags in PCI config space in default value
> (might be LPM enabled).
> 2.1. If runtime PM is disabled before first runtime suspend -
> registers will be left in their default state.
> 2.2. If runtime PM is disabled after first runtime suspend, the device
> will be woken up and the gl9763e runtime resume callback will disable
> LPM.

OK. Thank you for your answer.

Best Regards,
Ben Chuang

> >
> > >
> > > And the LPM negotiation flags look correct.
> > > Does that address your concerns?
> >
> > Best regards,
> > Ben Chuang
>
> --
> Best Regards,
> Stanisław Kardach

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

[Ben Chuang]

Hi Sven van Ashbrook,

On Thu, Aug 31, 2023 at 4:14 AM Sven van Ashbrook <svenva@chromium.org> wrote:
>
> On Tue, Aug 29, 2023 at 10:27 PM Ben Chuang <benchuanggli@gmail.com> wrote:
> >
> > >
> > > - if /sys/devices/.../power/control is "on", then:
> > >   <snip>
> > >
> > In this cas, after gl9763e_resume(), it is LPM disabled.
> > Is there no chance for gl9763e to enter L1.x again when the system is idle?
> >
>
> AFAIK the only way to disable runtime_pm is to write:
>   $ echo on > /sys/devices/.../power/control
> where
>   $ echo auto > /sys/devices/.../power/control
>       means: runtime_pm is actively managing the device, device can be "active"
>       or "suspended".
>   $ echo on > /sys/devices/.../power/control
>       means: runtime_pm is not managing the device, device is "active" only.
>
> In the "auto" case, we know what should happen: LPM negotiation is enabled when
> idle, disabled when active.
>
> What should be the LPM negotiation state in the "on" case? We have to
> make a choice:
> a) LPM negotiation disabled: normal performance, high power consumption, OR
> b) LPM negotiation  enabled: low    performance, low  power consumption
>
> If userspace disables our device's runtime_pm by writing "on", it expects the
> device to be always-on. It should then expect a higher power consumption.
> It should then also expect a performance that is not-worse than the "auto" case.
>
> So my suggestion would be to use (a), which is what this patch does.

Understood, I accept your suggestion.

>
> Appreciate your thoughts.

Best regards,
Ben Chuang

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

[Sven van Ashbrook]

On Wed, Aug 30, 2023 at 8:34 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> Understood, I accept your suggestion.
>

Thanks so much for the detailed review, much appreciated !
I shall be posting a v3 later this week. I look forward to Genesys's
review and/or Acked-by.

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

[Adrian Hunter]

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

[Sven van Ashbrook]

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 !