[RFC PATCH 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767

[RFC PATCH 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767

[Georg Gottleuber]

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@tuxedo.de>
Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>
Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com>
---
 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);
+
+       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);
+}
+
 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

Re: [RFC PATCH 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767

[Ben Chuang]

From: benchuanggli@gmail.com

[Resend email format, Sorry.]

Hi Georg and Christoffer,

On Tue, Oct 15, 2024 at 8:47 PM Georg Gottleuber <g.gottleuber@tuxedocomputers.com> 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@tuxedo.de>
> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>
> Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com>
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

Re: [RFC PATCH 1/1] mmc: sdhci-pci-gli: fix x86/S0ix SoCs suspend for GL9767

[Georg Gottleuber]

Hi Ben,

thank you for the quick reply.

Am 16.10.24 um 04:39 schrieb Ben Chuang:
> From: benchuanggli@gmail.com
> 
> [Resend email format, Sorry.]
> 
> Hi Georg and Christoffer,
> 
> On Tue, Oct 15, 2024 at 8:47 PM Georg Gottleuber <g.gottleuber@tuxedocomputers.com> 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@tuxedo.de>
>> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>
>> Signed-off-by: Georg Gottleuber <ggo@tuxedocomputers.com>
> Maybe need a Fixes: f3a5b56c1286 ("mmc: sdhci-pci-gli: Add Genesys Logic GL9767 support")

I was perhaps too cautious here.

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

Yes. Will fix it in v2.

>> +
>> +       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().

Yes. Will fix it in v2.

> 
>> +}
>> +
>>  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.

Yes, high power suspend only happens with (this single) Intel model. AMD
version sleeps well. (We only have these two models with GL9767.)

Unfortunately, we do not have a logic analyzer available. Our test
script reads following files and calculates percentage in relation to
sleep time:

/sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us

Additionally I measure during suspend with the battery disconnected and
USB-C measuring adapter.

Kind regards,
Georg Gottleuber