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

From: Adrian Hunter
Date: Fri Sep 01 2023 - 08:41:00 EST

On 1/09/23 09:33, Lai Jason wrote:
> 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.

The block device is a child of the host controller so must already be
suspended at this point. So there is no I/O in process.

cqhci_suspend() just turns the cmdq engine off.

So I don't think a change is needed, but please correct me if
I am wrong.

>
> 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
>>