Hi Ben,
thank you for the quick reply.
Am 16.10.24 um 04:39 schrieb Ben Chuang:
> 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")
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
