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
