From: benchuanggli@xxxxxxxxx [Resend email format, Sorry.] Hi Georg and Christoffer, On Tue, Oct 15, 2024 at 8:47 PM Georg Gottleuber <g.gottleuber@xxxxxxxxxxxxxxxxxxx> wrote: > > Adapt commit 1202d617e3d04c ("mmc: sdhci-pci-gli: fix LPM negotiation > so x86/S0ix SoCs can suspend") also for GL9767 card reader. > > This patch was written without specs or deeper knowledge of PCI sleep > states. Tests show that S0ix is reached and a lower power consumption > when suspended (6 watts vs 1.2 watts for TUXEDO InfinityBook Pro Gen9 > Intel). > > The function of the card reader appears to be normal. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=219284 > Co-developed-by: Christoffer Sandberg <cs@xxxxxxxxx> > Signed-off-by: Christoffer Sandberg <cs@xxxxxxxxx> > Signed-off-by: Georg Gottleuber <ggo@xxxxxxxxxxxxxxxxxxx> Maybe need a Fixes: f3a5b56c1286 ("mmc: sdhci-pci-gli: Add Genesys Logic GL9767 support") > --- > drivers/mmc/host/sdhci-pci-gli.c | 65 +++++++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c > b/drivers/mmc/host/sdhci-pci-gli.c > index 0f81586a19df..40f43f5cd5c7 100644 > --- a/drivers/mmc/host/sdhci-pci-gli.c > +++ b/drivers/mmc/host/sdhci-pci-gli.c > @@ -1205,6 +1205,32 @@ static void > gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, > pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value); > } > > +static void gl9767_set_low_power_negotiation(struct sdhci_pci_slot *slot, > + bool enable) > +{ > + struct pci_dev *pdev = slot->chip->pdev; > + u32 value; > + > + pci_read_config_dword(pdev, PCIE_GLI_9767_VHS, &value); > + value &= ~GLI_9767_VHS_REV; > + value |= FIELD_PREP(GLI_9767_VHS_REV, GLI_9767_VHS_REV_W); > + pci_write_config_dword(pdev, PCIE_GLI_9767_VHS, value); Maybe replace it with gl9767_vhs_write(). There are two functions gl9767_vhs_write()/gl9767_vhs_read() and they should be meant to be Vendor Header Space (VHS) writable/readable. > + > + pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value); > + > + if (enable) > + value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF; > + else > + value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF; > + > + pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); > + > + pci_read_config_dword(pdev, PCIE_GLI_9767_VHS, &value); > + value &= ~GLI_9767_VHS_REV; > + value |= FIELD_PREP(GLI_9767_VHS_REV, GLI_9767_VHS_REV_R); > + pci_write_config_dword(pdev, PCIE_GLI_9767_VHS, value); Maybe replace it with gl9767_vhs_read(). > +} > + > static void sdhci_set_gl9763e_signaling(struct sdhci_host *host, > unsigned int timing) > { > @@ -1470,6 +1496,42 @@ static int gl9763e_suspend(struct sdhci_pci_chip > *chip) > gl9763e_set_low_power_negotiation(slot, false); > return ret; > } > + > +static int gl9767_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; > + > + gl9767_set_low_power_negotiation(slot, false); > + > + return 0; > +} > + > +static int gl9767_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. > + */ > + gl9767_set_low_power_negotiation(slot, true); > + > + ret = sdhci_suspend_host(slot->host); > + if (ret) { > + gl9767_set_low_power_negotiation(slot, false); > + return ret; > + } > + > + return 0; > +} > #endif > > static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot) > @@ -1605,6 +1667,7 @@ const struct sdhci_pci_fixes sdhci_gl9767 = { > .probe_slot = gli_probe_slot_gl9767, > .ops = &sdhci_gl9767_ops, > #ifdef CONFIG_PM_SLEEP > - .resume = sdhci_pci_gli_resume, > + .resume = gl9767_resume, > + .suspend = gl9767_suspend, > #endif > }; > -- > 2.34.1 > Bugzilla wrote that this issue only happens on Intel models, right? How do you confirm the status of L1/L1SS, measuring PCIe link state via hardware or software? Thanks. Best regards, Ben Chuang