| Message ID | 41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767 | expand |
Hi, Georg I noticed that you mentioned in the patch for version 1 that the AMD platform is fine, may I ask that are AMD platforms and Intel platforms using the same kernel version? If they are using the same kernel version, how do you confirm that the solution is to modify the kernel code? Thanks, Victor Shih > -----Original Message----- > From: Georg Gottleuber <ggo@tuxedocomputers.com> > Sent: Thursday, October 17, 2024 4:11 PM > To: Ben Chuang <benchuanggli@gmail.com> > Cc: Ben Chuang <benchuanggli@gmail.com>; adrian.hunter@intel.com; Ulf Hansson <ulf.hansson@linaro.org>; VictorShih[施勝文] <Victor.Shih@genesyslogic.com.tw>; LucasLai[賴宗清] <Lucas.Lai@genesyslogic.com.tw>; GregTu[杜啟軒] <Greg.Tu@genesyslogic.com.tw>; HLLiu[劉鴻霖] <HL.Liu@genesyslogic.com.tw>; cs@tuxedo.de; Georg Gottleuber <ggo@tuxedocomputers.com>; BenChuang[莊智量] <Ben.Chuang@genesyslogic.com.tw>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [RFC PATCH v2 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767 > > [Some people who received this message don't often get email from ggo@tuxedocomputers.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > 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 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 OK. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=219284 > Fixes: f3a5b56c1286 ("mmc: sdhci-pci-gli: Add Genesys Logic GL9767 support") > Co-developed-by: Christoffer Sandberg <cs@tuxedo.de> > Signed-off-by: Christoffer Sandberg <cs@tuxedo.de> > Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com> > --- > v1 -> v2: > - use gl9767_vhs_read() and gl9767_vhs_write() > - editorial changes to the commit message > > drivers/mmc/host/sdhci-pci-gli.c | 61 +++++++++++++++++++++++++++++++- > 1 file changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c > b/drivers/mmc/host/sdhci-pci-gli.c > index 0f81586a19df..bdccd74bacd2 100644 > --- a/drivers/mmc/host/sdhci-pci-gli.c > +++ b/drivers/mmc/host/sdhci-pci-gli.c > @@ -1205,6 +1205,28 @@ 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; > + > + gl9767_vhs_write(pdev); > + > + pci_write_config_dword(pdev, PCIE_GLI_9767_VHS, value); > + > + 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); > + > + gl9767_vhs_read(pdev); > +} > + > static void sdhci_set_gl9763e_signaling(struct sdhci_host *host, > unsigned int timing) { @@ -1470,6 +1492,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 +1663,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 > ________________________________ > > Genesys Logic Email Confidentiality Notice: > This mail and any attachments may contain information that is confidential, proprietary, privileged or otherwise protected by law. The mail is intended solely for the named addressee (or a person responsible for delivering it to the addressee). If you are not the intended recipient of this mail, you are not authorized to read, print, copy or disseminate this mail. > > If you have received this email in error, please notify us immediately by reply email and immediately delete this message and any attachments from your system. Please be noted that any unauthorized use, dissemination, distribution or copying of this email is strictly prohibited. > ________________________________
Hi Victor, Am 17.10.24 um 17:05 schrieb Victor Shih: > Hi, Georg > > I noticed that you mentioned in the patch for version 1 that the AMD > platform is fine, > may I ask that are AMD platforms and Intel platforms using the same > kernel version? yes, they use the same kernel version. > If they are using the same kernel version, how do you confirm that > the solution is to modify the kernel code? I think the problem is similar to the one fixed by commit 1202d617e3d04c ("mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend") from Sven van Ashbrook. Unfortunately, I only have two devices with GL9767. I don't know if it really only affects Intel, but Sven van Ashbrook wrote in his commit: "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." That sounds like AMD is implementing the suspend differently. Additional experience / tests with GL9767 on Intel and AMD hardware would be helpful. Kind regards, Georg >> -----Original Message----- >> From: Georg Gottleuber <ggo@tuxedocomputers.com> >> Sent: Thursday, October 17, 2024 4:11 PM >> To: Ben Chuang <benchuanggli@gmail.com> >> Cc: Ben Chuang <benchuanggli@gmail.com>; adrian.hunter@intel.com; Ulf Hansson <ulf.hansson@linaro.org>; VictorShih[施勝文] <Victor.Shih@genesyslogic.com.tw>; LucasLai[賴宗清] <Lucas.Lai@genesyslogic.com.tw>; GregTu[杜啟軒] <Greg.Tu@genesyslogic.com.tw>; HLLiu[劉鴻霖] <HL.Liu@genesyslogic.com.tw>; cs@tuxedo.de; Georg Gottleuber <ggo@tuxedocomputers.com>; BenChuang[莊智量] <Ben.Chuang@genesyslogic.com.tw>; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: [RFC PATCH v2 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767 >> >> [Some people who received this message don't often get email from ggo@tuxedocomputers.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >> >> 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 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 OK. >> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219284 >> Fixes: f3a5b56c1286 ("mmc: sdhci-pci-gli: Add Genesys Logic GL9767 support") >> Co-developed-by: Christoffer Sandberg <cs@tuxedo.de> >> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de> >> Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com> >> --- >> v1 -> v2: >> - use gl9767_vhs_read() and gl9767_vhs_write() >> - editorial changes to the commit message >> >> drivers/mmc/host/sdhci-pci-gli.c | 61 +++++++++++++++++++++++++++++++- >> 1 file changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-pci-gli.c >> b/drivers/mmc/host/sdhci-pci-gli.c >> index 0f81586a19df..bdccd74bacd2 100644 >> --- a/drivers/mmc/host/sdhci-pci-gli.c >> +++ b/drivers/mmc/host/sdhci-pci-gli.c >> @@ -1205,6 +1205,28 @@ 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; >> + >> + gl9767_vhs_write(pdev); >> + >> + pci_write_config_dword(pdev, PCIE_GLI_9767_VHS, value); >> + >> + 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); >> + >> + gl9767_vhs_read(pdev); >> +} >> + >> static void sdhci_set_gl9763e_signaling(struct sdhci_host *host, >> unsigned int timing) { @@ -1470,6 +1492,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 +1663,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
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 0f81586a19df..bdccd74bacd2 100644 --- a/drivers/mmc/host/sdhci-pci-gli.c +++ b/drivers/mmc/host/sdhci-pci-gli.c @@ -1205,6 +1205,28 @@ 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; + + gl9767_vhs_write(pdev); + + pci_write_config_dword(pdev, PCIE_GLI_9767_VHS, value); + + 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); + + gl9767_vhs_read(pdev); +} + static void sdhci_set_gl9763e_signaling(struct sdhci_host *host, unsigned int timing)
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 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 OK. Link: https://bugzilla.kernel.org/show_bug.cgi?id=219284 Fixes: f3a5b56c1286 ("mmc: sdhci-pci-gli: Add Genesys Logic GL9767 support") Co-developed-by: Christoffer Sandberg <cs@tuxedo.de> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de> Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com> --- v1 -> v2: - use gl9767_vhs_read() and gl9767_vhs_write() - editorial changes to the commit message drivers/mmc/host/sdhci-pci-gli.c | 61 +++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) { @@ -1470,6 +1492,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 +1663,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 };