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@xxxxxxxxx <mailto:adrian.hunter@xxxxxxxxx>> 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@xxxxxxxxxx <mailto:skardach@xxxxxxxxxx>>
>     > Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
>     > Cc: stable@xxxxxxxxxxxxxxx <mailto:stable@xxxxxxxxxxxxxxx>
>     > Signed-off-by: Sven van Ashbrook <svenva@xxxxxxxxxxxx <mailto:svenva@xxxxxxxxxxxx>>
>     >      # 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