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

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

sdhci_pci module is blocking low power s0ix sleep with GL9767

[Georg Gottleuber]

Hello,

I have a problem with s0ix sleep on a laptop with GL9767 SD card reader.

The device is a TUXEDO InfinityBook Pro 14 Gen9 Intel laptop and 
consumes 6 watts in s2idle (not reaching s0ix). If I blacklist sdhci_pci 
in /etc/modprobe.d/blacklist.conf then the device sleeps with 1.2 watts 
(this is not super good, but OK). Unfortunately unloading and reloading 
does not work either. Once the module has been loaded, the high power 
consumption remains, even after a rmmod.

I tested this behavior with linux mainline 6.11, 
6.11.0-rc7-next-20240909. Kernel mmc/next does not work either.

In an AMD device (InfinityBook Pro 14 Gen9 AMD), however, the same card 
reader sleeps without any problems.

Link https://bugzilla.kernel.org/show_bug.cgi?id=219284

Any ideas?

Kind regards
Georg Gottleuber

Re: sdhci_pci module is blocking low power s0ix sleep with GL9767

[Ulf Hansson]

+ Ben

On Thu, 26 Sept 2024 at 17:31, Georg Gottleuber <ggo@tuxedocomputers.com> wrote:
>
> Hello,
>
> I have a problem with s0ix sleep on a laptop with GL9767 SD card reader.
>
> The device is a TUXEDO InfinityBook Pro 14 Gen9 Intel laptop and
> consumes 6 watts in s2idle (not reaching s0ix). If I blacklist sdhci_pci
> in /etc/modprobe.d/blacklist.conf then the device sleeps with 1.2 watts
> (this is not super good, but OK). Unfortunately unloading and reloading
> does not work either. Once the module has been loaded, the high power
> consumption remains, even after a rmmod.
>
> I tested this behavior with linux mainline 6.11,
> 6.11.0-rc7-next-20240909. Kernel mmc/next does not work either.
>
> In an AMD device (InfinityBook Pro 14 Gen9 AMD), however, the same card
> reader sleeps without any problems.
>
> Link https://bugzilla.kernel.org/show_bug.cgi?id=219284
>
> Any ideas?

I am sorry, but I don't have any useful suggestions at this point. We
need some help from the guys from Genesys Logic.

>
> Kind regards
> Georg Gottleuber

Kind regards
Uffe

[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