| Message ID | 20230831160055.v3.1.I7ed1ca09797be2dd76ca914c57d88b32d24dac88@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend | expand |
Hi, On Fri, Sep 1, 2023 at 12:01 AM Sven van Ashbrook <svenva@chromium.org> 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@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> 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@mail.gmail.com/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 >
On 1/09/23 09:33, Lai Jason wrote: > Hi, > > On Fri, Sep 1, 2023 at 12:01 AM Sven van Ashbrook <svenva@chromium.org> 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@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> > > 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@mail.gmail.com/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 >>
On 31/08/23 19:00, 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 > 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@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> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > > Changes in v3: > - applied maintainer feedback from https://lore.kernel.org/lkml/CACT4zj-BaX4tHji8B8gS5jiKkd-2BcwfzHM4fS-OUn0f8DSxcw@mail.gmail.com/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,
What do we need for Ulf to add this to the maintainer git? There are released devices waiting for this fix, but picking from list generates lots of paperwork, so I'd prefer to pick from git. We have a LGTM from Jason Lai, do we need one from Ben Chuang as well? On Mon, Sep 4, 2023 at 3:42 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> >
From: benchuanggli@gmail.com On Tue, Sep 5, 2023 at 11:15 AM Sven van Ashbrook <svenva@chromium.org> > What do we need for Ulf to add this to the maintainer git? There are > released devices waiting for this fix, but picking from list generates > lots of paperwork, so I'd prefer to pick from git. > > We have a LGTM from Jason Lai, do we need one from Ben Chuang as well? LGTM, too. Best regards, Ben Chuang > >On Mon, Sep 4, 2023 at 3:42 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >>
On 5/09/23 21:15, Sven van Ashbrook wrote: > What do we need for Ulf to add this to the maintainer git? Nothing, it will get processed in due course, likely making it into one of the 6.6 release candidates, because it is a fix, and from there to stable. But right now is the middle of the merge window so some delay can be expected anyway. > What do we need for Ulf to add this to the maintainer git? There are > There are > released devices waiting for this fix, but picking from list generates > lots of paperwork, so I'd prefer to pick from git. > > We have a LGTM from Jason Lai, do we need one from Ben Chuang as well? > > On Mon, Sep 4, 2023 at 3:42 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >>
On Thu, 31 Aug 2023 at 18:01, Sven van Ashbrook <svenva@chromium.org> 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@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> Applied for fixes, thanks! Kind regards Uffe > --- > > Changes in v3: > - applied maintainer feedback from https://lore.kernel.org/lkml/CACT4zj-BaX4tHji8B8gS5jiKkd-2BcwfzHM4fS-OUn0f8DSxcw@mail.gmail.com/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 >
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,
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@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> --- Changes in v3: - applied maintainer feedback from https://lore.kernel.org/lkml/CACT4zj-BaX4tHji8B8gS5jiKkd-2BcwfzHM4fS-OUn0f8DSxcw@mail.gmail.com/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(-)