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
