| Message ID | 20231114115516.1585361-1-korneld@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mmc: sdhci-pci-gli: Disable LPM during initialization | expand |
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 >
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];
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 >
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];
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(-)