Hi,
On Fri, Sep 1, 2023 at 12:01 AM Sven van Ashbrook <svenva@xxxxxxxxxxxx> wrote:
>
> To improve the r/w performance of GL9763E, the current driver inhibits LPM
> negotiation while the device is active.
>
> This prevents a large number of SoCs from suspending, notably x86 systems
> which commonly use S0ix as the suspend mechanism - for example, Intel
> Alder Lake and Raptor Lake processors.
>
> Failure description:
> 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>
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sven van Ashbrook <svenva@xxxxxxxxxxxx>
LGTM.
But I prefer Ben's opinion "I suppose cqhci_suspend() may need to be
done first safely, then
gl9763e_set_low_power_negotiation(slot, true)." because it can avoid
GL9763E entering L1
while cmdq engine processes I/O.
Best regards,
Jason Lai
> ---
>
> Changes in v3:
> - applied maintainer feedback from https://lore.kernel.org/lkml/CACT4zj-BaX4tHji8B8gS5jiKkd-2BcwfzHM4fS-OUn0f8DSxcw@xxxxxxxxxxxxxx/T/#m7cea7b6b987d1ab1ca95feedf2c6f9da5783da5c
>
> Changes in v2:
> - improved symmetry and error path in s2idle suspend callback (internal review)
>
> drivers/mmc/host/sdhci-pci-gli.c | 104 ++++++++++++++++++++-----------
> 1 file changed, 66 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 1792665c9494a..a4ccb6c3e27a6 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -745,42 +745,6 @@ static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
> return value;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip)
> -{
> - struct sdhci_pci_slot *slot = chip->slots[0];
> -
> - pci_free_irq_vectors(slot->chip->pdev);
> - gli_pcie_enable_msi(slot);
> -
> - return sdhci_pci_resume_host(chip);
> -}
> -
> -static int sdhci_cqhci_gli_resume(struct sdhci_pci_chip *chip)
> -{
> - struct sdhci_pci_slot *slot = chip->slots[0];
> - int ret;
> -
> - ret = sdhci_pci_gli_resume(chip);
> - if (ret)
> - return ret;
> -
> - return cqhci_resume(slot->host->mmc);
> -}
> -
> -static int sdhci_cqhci_gli_suspend(struct sdhci_pci_chip *chip)
> -{
> - struct sdhci_pci_slot *slot = chip->slots[0];
> - int ret;
> -
> - ret = cqhci_suspend(slot->host->mmc);
> - if (ret)
> - return ret;
> -
> - return sdhci_suspend_host(slot->host);
> -}
> -#endif
> -
> static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
> struct mmc_ios *ios)
> {
> @@ -1029,6 +993,70 @@ static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip)
> }
> #endif
>
> +#ifdef CONFIG_PM_SLEEP
> +static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip)
> +{
> + struct sdhci_pci_slot *slot = chip->slots[0];
> +
> + pci_free_irq_vectors(slot->chip->pdev);
> + gli_pcie_enable_msi(slot);
> +
> + return sdhci_pci_resume_host(chip);
> +}
> +
> +static int gl9763e_resume(struct sdhci_pci_chip *chip)
> +{
> + struct sdhci_pci_slot *slot = chip->slots[0];
> + int ret;
> +
> + ret = sdhci_pci_gli_resume(chip);
> + if (ret)
> + return ret;
> +
> + ret = cqhci_resume(slot->host->mmc);
> + if (ret)
> + return ret;
> +
> + /*
> + * Disable LPM negotiation to bring device back in sync
> + * with its runtime_pm state.
> + */
> + gl9763e_set_low_power_negotiation(slot, false);
> +
> + return 0;
> +}
> +
> +static int gl9763e_suspend(struct sdhci_pci_chip *chip)
> +{
> + struct sdhci_pci_slot *slot = chip->slots[0];
> + int ret;
> +
> + /*
> + * Certain SoCs can suspend only with the bus in low-
> + * power state, notably x86 SoCs when using S0ix.
> + * Re-enable LPM negotiation to allow entering L1 state
> + * and entering system suspend.
> + */
> + gl9763e_set_low_power_negotiation(slot, true);
> +
> + ret = cqhci_suspend(slot->host->mmc);
> + if (ret)
> + goto err_suspend;
> +
> + ret = sdhci_suspend_host(slot->host);
> + if (ret)
> + goto err_suspend_host;
> +
> + return 0;
> +
> +err_suspend_host:
> + cqhci_resume(slot->host->mmc);
> +err_suspend:
> + gl9763e_set_low_power_negotiation(slot, false);
> + return ret;
> +}
> +#endif
> +
> static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot)
> {
> struct pci_dev *pdev = slot->chip->pdev;
> @@ -1113,8 +1141,8 @@ const struct sdhci_pci_fixes sdhci_gl9763e = {
> .probe_slot = gli_probe_slot_gl9763e,
> .ops = &sdhci_gl9763e_ops,
> #ifdef CONFIG_PM_SLEEP
> - .resume = sdhci_cqhci_gli_resume,
> - .suspend = sdhci_cqhci_gli_suspend,
> + .resume = gl9763e_resume,
> + .suspend = gl9763e_suspend,
> #endif
> #ifdef CONFIG_PM
> .runtime_suspend = gl9763e_runtime_suspend,
> --
> 2.42.0.283.g2d96d420d3-goog
>
