* [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization
@ 2023-11-14 11:54 Kornel Dulęba
2023-11-14 13:01 ` Sven van Ashbrook
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Kornel Dulęba @ 2023-11-14 11:54 UTC (permalink / raw)
To: Ulf Hansson, Adrian Hunter, Sven van Ashbrook, Jason Lai
Cc: Victor Shih, Ben Chuang, Stanisław Kardach, linux-kernel,
linux-mmc, Kornel Dulęba, stable
To address IO performance commit f9e5b33934ce
("mmc: host: Improve I/O read/write performance for GL9763E")
limited LPM negotiation to runtime suspend state.
The problem is that it only flips the switch in the runtime PM
resume/suspend logic.
Disable LPM negotiation in gl9763e_add_host.
This helps in two ways:
1. It was found that the LPM switch stays in the same position after
warm reboot. Having it set in init helps with consistency.
2. Disabling LPM during the first runtime resume leaves us susceptible
to the performance issue in the time window between boot and the
first runtime suspend.
Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
Cc: stable@vger.kernel.org
Signed-off-by: Kornel Dulęba <korneld@chromium.org>
---
v2: Move up gl9763e_set_low_power_negotiation to avoid having to forward
declare it.
drivers/mmc/host/sdhci-pci-gli.c | 54 +++++++++++++++++---------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index d8a991b349a8..77911a57b12c 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -1189,6 +1189,32 @@ static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
sdhci_writel(host, val, SDHCI_GLI_9763E_HS400_ES_REG);
}
+static void gl9763e_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_9763E_VHS, &value);
+ value &= ~GLI_9763E_VHS_REV;
+ value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
+ pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
+
+ pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
+
+ if (enable)
+ value &= ~GLI_9763E_CFG_LPSN_DIS;
+ else
+ value |= GLI_9763E_CFG_LPSN_DIS;
+
+ pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
+
+ pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
+ value &= ~GLI_9763E_VHS_REV;
+ value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
+ pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
+}
+
static void sdhci_set_gl9763e_signaling(struct sdhci_host *host,
unsigned int timing)
{
@@ -1297,6 +1323,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
if (ret)
goto cleanup;
+ /* Disable LPM negotiation to avoid entering L1 state. */
+ gl9763e_set_low_power_negotiation(slot, false);
+
return 0;
cleanup:
@@ -1340,31 +1369,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
}
#ifdef CONFIG_PM
-static void gl9763e_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_9763E_VHS, &value);
- value &= ~GLI_9763E_VHS_REV;
- value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
- pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
-
- pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
-
- if (enable)
- value &= ~GLI_9763E_CFG_LPSN_DIS;
- else
- value |= GLI_9763E_CFG_LPSN_DIS;
-
- pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
-
- pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
- value &= ~GLI_9763E_VHS_REV;
- value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
- pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
-}
-
static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
{
struct sdhci_pci_slot *slot = chip->slots[0];
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization
2023-11-14 11:54 [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization Kornel Dulęba
@ 2023-11-14 13:01 ` Sven van Ashbrook
2023-11-20 13:33 ` Adrian Hunter
2023-11-23 17:18 ` Ulf Hansson
2 siblings, 0 replies; 4+ messages in thread
From: Sven van Ashbrook @ 2023-11-14 13:01 UTC (permalink / raw)
To: Kornel Dulęba
Cc: Ulf Hansson, Adrian Hunter, Jason Lai, Victor Shih, Ben Chuang,
Stanisław Kardach, linux-kernel, linux-mmc, stable
LGTM.
Reviewed-by: Sven van Ashbrook <svenva@chromium.org>
On Tue, Nov 14, 2023 at 6:56 AM Kornel Dulęba <korneld@chromium.org> wrote:
>
> To address IO performance commit f9e5b33934ce
> ("mmc: host: Improve I/O read/write performance for GL9763E")
> limited LPM negotiation to runtime suspend state.
> The problem is that it only flips the switch in the runtime PM
> resume/suspend logic.
>
> Disable LPM negotiation in gl9763e_add_host.
> This helps in two ways:
> 1. It was found that the LPM switch stays in the same position after
> warm reboot. Having it set in init helps with consistency.
> 2. Disabling LPM during the first runtime resume leaves us susceptible
> to the performance issue in the time window between boot and the
> first runtime suspend.
>
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> ---
> v2: Move up gl9763e_set_low_power_negotiation to avoid having to forward
> declare it.
>
> drivers/mmc/host/sdhci-pci-gli.c | 54 +++++++++++++++++---------------
> 1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d8a991b349a8..77911a57b12c 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -1189,6 +1189,32 @@ static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
> sdhci_writel(host, val, SDHCI_GLI_9763E_HS400_ES_REG);
> }
>
> +static void gl9763e_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_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> +
> + if (enable)
> + value &= ~GLI_9763E_CFG_LPSN_DIS;
> + else
> + value |= GLI_9763E_CFG_LPSN_DIS;
> +
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +}
> +
> static void sdhci_set_gl9763e_signaling(struct sdhci_host *host,
> unsigned int timing)
> {
> @@ -1297,6 +1323,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
> if (ret)
> goto cleanup;
>
> + /* Disable LPM negotiation to avoid entering L1 state. */
> + gl9763e_set_low_power_negotiation(slot, false);
> +
> return 0;
>
> cleanup:
> @@ -1340,31 +1369,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
> }
>
> #ifdef CONFIG_PM
> -static void gl9763e_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_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> -
> - if (enable)
> - value &= ~GLI_9763E_CFG_LPSN_DIS;
> - else
> - value |= GLI_9763E_CFG_LPSN_DIS;
> -
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -}
> -
> static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
> {
> struct sdhci_pci_slot *slot = chip->slots[0];
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization
2023-11-14 11:54 [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization Kornel Dulęba
2023-11-14 13:01 ` Sven van Ashbrook
@ 2023-11-20 13:33 ` Adrian Hunter
2023-11-23 17:18 ` Ulf Hansson
2 siblings, 0 replies; 4+ messages in thread
From: Adrian Hunter @ 2023-11-20 13:33 UTC (permalink / raw)
To: Kornel Dulęba, Ulf Hansson, Sven van Ashbrook, Jason Lai
Cc: Victor Shih, Ben Chuang, Stanisław Kardach, linux-kernel,
linux-mmc, stable
On 14/11/23 13:54, Kornel Dulęba wrote:
> To address IO performance commit f9e5b33934ce
> ("mmc: host: Improve I/O read/write performance for GL9763E")
> limited LPM negotiation to runtime suspend state.
> The problem is that it only flips the switch in the runtime PM
> resume/suspend logic.
>
> Disable LPM negotiation in gl9763e_add_host.
> This helps in two ways:
> 1. It was found that the LPM switch stays in the same position after
> warm reboot. Having it set in init helps with consistency.
> 2. Disabling LPM during the first runtime resume leaves us susceptible
> to the performance issue in the time window between boot and the
> first runtime suspend.
>
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> v2: Move up gl9763e_set_low_power_negotiation to avoid having to forward
> declare it.
>
> drivers/mmc/host/sdhci-pci-gli.c | 54 +++++++++++++++++---------------
> 1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d8a991b349a8..77911a57b12c 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -1189,6 +1189,32 @@ static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
> sdhci_writel(host, val, SDHCI_GLI_9763E_HS400_ES_REG);
> }
>
> +static void gl9763e_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_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> +
> + if (enable)
> + value &= ~GLI_9763E_CFG_LPSN_DIS;
> + else
> + value |= GLI_9763E_CFG_LPSN_DIS;
> +
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +}
> +
> static void sdhci_set_gl9763e_signaling(struct sdhci_host *host,
> unsigned int timing)
> {
> @@ -1297,6 +1323,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
> if (ret)
> goto cleanup;
>
> + /* Disable LPM negotiation to avoid entering L1 state. */
> + gl9763e_set_low_power_negotiation(slot, false);
> +
> return 0;
>
> cleanup:
> @@ -1340,31 +1369,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
> }
>
> #ifdef CONFIG_PM
> -static void gl9763e_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_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> -
> - if (enable)
> - value &= ~GLI_9763E_CFG_LPSN_DIS;
> - else
> - value |= GLI_9763E_CFG_LPSN_DIS;
> -
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -}
> -
> static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
> {
> struct sdhci_pci_slot *slot = chip->slots[0];
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization
2023-11-14 11:54 [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization Kornel Dulęba
2023-11-14 13:01 ` Sven van Ashbrook
2023-11-20 13:33 ` Adrian Hunter
@ 2023-11-23 17:18 ` Ulf Hansson
2 siblings, 0 replies; 4+ messages in thread
From: Ulf Hansson @ 2023-11-23 17:18 UTC (permalink / raw)
To: Kornel Dulęba
Cc: Adrian Hunter, Sven van Ashbrook, Jason Lai, Victor Shih,
Ben Chuang, Stanisław Kardach, linux-kernel, linux-mmc,
stable
On Tue, 14 Nov 2023 at 12:55, Kornel Dulęba <korneld@chromium.org> wrote:
>
> To address IO performance commit f9e5b33934ce
> ("mmc: host: Improve I/O read/write performance for GL9763E")
> limited LPM negotiation to runtime suspend state.
> The problem is that it only flips the switch in the runtime PM
> resume/suspend logic.
>
> Disable LPM negotiation in gl9763e_add_host.
> This helps in two ways:
> 1. It was found that the LPM switch stays in the same position after
> warm reboot. Having it set in init helps with consistency.
> 2. Disabling LPM during the first runtime resume leaves us susceptible
> to the performance issue in the time window between boot and the
> first runtime suspend.
>
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
Applied for fixes, thanks!
Kind regards
Uffe
> ---
> v2: Move up gl9763e_set_low_power_negotiation to avoid having to forward
> declare it.
>
> drivers/mmc/host/sdhci-pci-gli.c | 54 +++++++++++++++++---------------
> 1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d8a991b349a8..77911a57b12c 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -1189,6 +1189,32 @@ static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
> sdhci_writel(host, val, SDHCI_GLI_9763E_HS400_ES_REG);
> }
>
> +static void gl9763e_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_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> +
> + if (enable)
> + value &= ~GLI_9763E_CFG_LPSN_DIS;
> + else
> + value |= GLI_9763E_CFG_LPSN_DIS;
> +
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> +
> + pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> + value &= ~GLI_9763E_VHS_REV;
> + value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> + pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> +}
> +
> static void sdhci_set_gl9763e_signaling(struct sdhci_host *host,
> unsigned int timing)
> {
> @@ -1297,6 +1323,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
> if (ret)
> goto cleanup;
>
> + /* Disable LPM negotiation to avoid entering L1 state. */
> + gl9763e_set_low_power_negotiation(slot, false);
> +
> return 0;
>
> cleanup:
> @@ -1340,31 +1369,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
> }
>
> #ifdef CONFIG_PM
> -static void gl9763e_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_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_W);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &value);
> -
> - if (enable)
> - value &= ~GLI_9763E_CFG_LPSN_DIS;
> - else
> - value |= GLI_9763E_CFG_LPSN_DIS;
> -
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG, value);
> -
> - pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
> - value &= ~GLI_9763E_VHS_REV;
> - value |= FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763E_VHS_REV_R);
> - pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
> -}
> -
> static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
> {
> struct sdhci_pci_slot *slot = chip->slots[0];
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-23 17:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-14 11:54 [PATCH v2] mmc: sdhci-pci-gli: Disable LPM during initialization Kornel Dulęba
2023-11-14 13:01 ` Sven van Ashbrook
2023-11-20 13:33 ` Adrian Hunter
2023-11-23 17:18 ` Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox