diff mbox series

[RESEND] mmc: host: Improve READ/WRITE Performance of GL9763E

Message ID 20220602114815.1801-1-jasonlai.genesyslogic@gmail.com
State Superseded
Headers show
Series [RESEND] mmc: host: Improve READ/WRITE Performance of GL9763E | expand

Commit Message

Lai Jason June 2, 2022, 11:48 a.m. UTC
From: Jason Lai <jason.lai@genesyslogic.com.tw>

Resend this path to fix typo in the following message.

This patch is the follow-up to the patch [1] and adopt Ulf's comment.

Due to flaws in hardware design, GL9763E takes long time to exit from L1
state. The I/O performance will suffer severe impact if it often enter
and exit L1 state during I/O requests.

To improve READ/WRITE performance and take battery life into account, we
turn on GL9763E L1 negotiation before entering runtime suspend and turn
off GL9763E L1 negotiation while executing runtime resume. That is to say,
GL9763E will not enter L1 state when executing I/O requests and enter L1 
state when PCIe bus idle.

[1] https://patchwork.kernel.org/project/linux-mmc/list/?series=645922

Signed-off-by: Renius Chen <reniuschengl@gmail.com>
Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
---
 drivers/mmc/host/sdhci-pci-gli.c | 57 +++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

Comments

Lai Jason June 7, 2022, 9:45 a.m. UTC | #1
Hi Ulf,
    Do you have any comments about this patch?

kind regards,
Jason Lai



On Thu, Jun 2, 2022 at 7:48 PM Jason Lai
<jasonlai.genesyslogic@gmail.com> wrote:
>
> From: Jason Lai <jason.lai@genesyslogic.com.tw>
>
> Resend this path to fix typo in the following message.
>
> This patch is the follow-up to the patch [1] and adopt Ulf's comment.
>
> Due to flaws in hardware design, GL9763E takes long time to exit from L1
> state. The I/O performance will suffer severe impact if it often enter
> and exit L1 state during I/O requests.
>
> To improve READ/WRITE performance and take battery life into account, we
> turn on GL9763E L1 negotiation before entering runtime suspend and turn
> off GL9763E L1 negotiation while executing runtime resume. That is to say,
> GL9763E will not enter L1 state when executing I/O requests and enter L1
> state when PCIe bus idle.
>
> [1] https://patchwork.kernel.org/project/linux-mmc/list/?series=645922
>
> Signed-off-by: Renius Chen <reniuschengl@gmail.com>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 57 +++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d09728c37d03..891bcd38fd6d 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -95,9 +95,12 @@
>  #define PCIE_GLI_9763E_SCR      0x8E0
>  #define   GLI_9763E_SCR_AXI_REQ           BIT(9)
>
> +#define PCIE_GLI_9763E_CFG       0x8A0
> +#define   GLI_9763E_CFG_LPSN_DIS   BIT(12)
> +
>  #define PCIE_GLI_9763E_CFG2      0x8A4
>  #define   GLI_9763E_CFG2_L1DLY     GENMASK(28, 19)
> -#define   GLI_9763E_CFG2_L1DLY_MID 0x54
> +#define   GLI_9763E_CFG2_L1DLY_MID 0x54                // Set L1 entry delay time to 21us
>
>  #define PCIE_GLI_9763E_MMC_CTRL  0x960
>  #define   GLI_9763E_HS400_SLOW     BIT(3)
> @@ -144,6 +147,10 @@
>
>  #define GLI_MAX_TUNING_LOOP 40
>
> +struct gli_host {
> +       bool lpm_negotiation_enabled;
> +};
> +
>  /* Genesys Logic chipset */
>  static inline void gl9750_wt_on(struct sdhci_host *host)
>  {
> @@ -818,6 +825,43 @@ static void sdhci_gl9763e_dumpregs(struct mmc_host *mmc)
>         sdhci_dumpregs(mmc_priv(mmc));
>  }
>
> +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 gl9763e_set_lpm_negotiation(struct sdhci_pci_slot *slot, bool enable)
> +{
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
> +
> +       if (gli_host->lpm_negotiation_enabled == enable)
> +               return;
> +
> +       gli_host->lpm_negotiation_enabled = enable;
> +
> +       gl9763e_set_low_power_negotiation(slot, enable);
> +}
> +
>  static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc)
>  {
>         struct cqhci_host *cq_host = mmc->cqe_private;
> @@ -921,6 +965,7 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>  {
>         struct pci_dev *pdev = slot->chip->pdev;
>         u32 value;
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
>
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
>         value &= ~GLI_9763E_VHS_REV;
> @@ -941,6 +986,9 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>         value |= FIELD_PREP(GLI_9763E_CFG2_L1DLY, GLI_9763E_CFG2_L1DLY_MID);
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG2, value);
>
> +       /* Default setting of LPM negotiation is enabled. */
> +       gli_host->lpm_negotiation_enabled = true;
> +
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_CLKRXDLY, &value);
>         value &= ~GLI_9763E_HS400_RXDLY;
>         value |= FIELD_PREP(GLI_9763E_HS400_RXDLY, GLI_9763E_HS400_RXDLY_5);
> @@ -959,6 +1007,9 @@ static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
>         struct sdhci_host *host = slot->host;
>         u16 clock;
>
> +       /* Enable LPM negotiatiom to allow entering L1 state */
> +       gl9763e_set_lpm_negotiation(slot, true);
> +
>         clock = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>         clock &= ~(SDHCI_CLOCK_PLL_EN | SDHCI_CLOCK_CARD_EN);
>         sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
> @@ -989,6 +1040,9 @@ static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip)
>         clock |= SDHCI_CLOCK_CARD_EN;
>         sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
>
> +       /* Disable LPM negotiatiom to avoid entering L1 state. */
> +       gl9763e_set_lpm_negotiation(slot, false);
> +
>         return 0;
>  }
>  #endif
> @@ -1109,4 +1163,5 @@ const struct sdhci_pci_fixes sdhci_gl9763e = {
>         .allow_runtime_pm = true,
>  #endif
>         .add_host       = gl9763e_add_host,
> +       .priv_size      = sizeof(struct gli_host),
>  };
> --
> 2.36.1
>
Ulf Hansson June 7, 2022, 11:25 a.m. UTC | #2
On Thu, 2 Jun 2022 at 13:48, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
>
> From: Jason Lai <jason.lai@genesyslogic.com.tw>
>
> Resend this path to fix typo in the following message.
>
> This patch is the follow-up to the patch [1] and adopt Ulf's comment.

Please start to care about versioning your patches correctly. It's
difficult to follow what goes on in between each submission, thus it
makes it harder for me to review.

Moreover, the above information is nice to have, but it should not be
a part of the commit message, but rather in the separate section. See
more below.

>
> Due to flaws in hardware design, GL9763E takes long time to exit from L1
> state. The I/O performance will suffer severe impact if it often enter
> and exit L1 state during I/O requests.
>
> To improve READ/WRITE performance and take battery life into account, we
> turn on GL9763E L1 negotiation before entering runtime suspend and turn
> off GL9763E L1 negotiation while executing runtime resume. That is to say,
> GL9763E will not enter L1 state when executing I/O requests and enter L1
> state when PCIe bus idle.
>
> [1] https://patchwork.kernel.org/project/linux-mmc/list/?series=645922
>
> Signed-off-by: Renius Chen <reniuschengl@gmail.com>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> ---

This is where patch versioning and other information belongs. To keep
the correct patch format, add a newline here and end the section with
three "dashes" on a separate line.

---
>  drivers/mmc/host/sdhci-pci-gli.c | 57 +++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d09728c37d03..891bcd38fd6d 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -95,9 +95,12 @@
>  #define PCIE_GLI_9763E_SCR      0x8E0
>  #define   GLI_9763E_SCR_AXI_REQ           BIT(9)
>
> +#define PCIE_GLI_9763E_CFG       0x8A0
> +#define   GLI_9763E_CFG_LPSN_DIS   BIT(12)
> +
>  #define PCIE_GLI_9763E_CFG2      0x8A4
>  #define   GLI_9763E_CFG2_L1DLY     GENMASK(28, 19)
> -#define   GLI_9763E_CFG2_L1DLY_MID 0x54
> +#define   GLI_9763E_CFG2_L1DLY_MID 0x54                // Set L1 entry delay time to 21us
>
>  #define PCIE_GLI_9763E_MMC_CTRL  0x960
>  #define   GLI_9763E_HS400_SLOW     BIT(3)
> @@ -144,6 +147,10 @@
>
>  #define GLI_MAX_TUNING_LOOP 40
>
> +struct gli_host {
> +       bool lpm_negotiation_enabled;
> +};
> +
>  /* Genesys Logic chipset */
>  static inline void gl9750_wt_on(struct sdhci_host *host)
>  {
> @@ -818,6 +825,43 @@ static void sdhci_gl9763e_dumpregs(struct mmc_host *mmc)
>         sdhci_dumpregs(mmc_priv(mmc));
>  }
>
> +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 gl9763e_set_lpm_negotiation(struct sdhci_pci_slot *slot, bool enable)
> +{
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
> +
> +       if (gli_host->lpm_negotiation_enabled == enable)
> +               return;

I don't think you need to keep track of the previous state.

When the host becomes runtime suspended we enable the lpm mode. When
the host is runtime resumed we disable the lpm mode. That looks
sufficient to me, no?

> +
> +       gli_host->lpm_negotiation_enabled = enable;
> +
> +       gl9763e_set_low_power_negotiation(slot, enable);
> +}
> +
>  static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc)
>  {
>         struct cqhci_host *cq_host = mmc->cqe_private;
> @@ -921,6 +965,7 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>  {
>         struct pci_dev *pdev = slot->chip->pdev;
>         u32 value;
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
>
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
>         value &= ~GLI_9763E_VHS_REV;
> @@ -941,6 +986,9 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>         value |= FIELD_PREP(GLI_9763E_CFG2_L1DLY, GLI_9763E_CFG2_L1DLY_MID);
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG2, value);
>
> +       /* Default setting of LPM negotiation is enabled. */
> +       gli_host->lpm_negotiation_enabled = true;
> +
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_CLKRXDLY, &value);
>         value &= ~GLI_9763E_HS400_RXDLY;
>         value |= FIELD_PREP(GLI_9763E_HS400_RXDLY, GLI_9763E_HS400_RXDLY_5);
> @@ -959,6 +1007,9 @@ static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
>         struct sdhci_host *host = slot->host;
>         u16 clock;
>
> +       /* Enable LPM negotiatiom to allow entering L1 state */
> +       gl9763e_set_lpm_negotiation(slot, true);
> +
>         clock = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>         clock &= ~(SDHCI_CLOCK_PLL_EN | SDHCI_CLOCK_CARD_EN);
>         sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
> @@ -989,6 +1040,9 @@ static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip)
>         clock |= SDHCI_CLOCK_CARD_EN;
>         sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
>
> +       /* Disable LPM negotiatiom to avoid entering L1 state. */
> +       gl9763e_set_lpm_negotiation(slot, false);
> +
>         return 0;
>  }
>  #endif
> @@ -1109,4 +1163,5 @@ const struct sdhci_pci_fixes sdhci_gl9763e = {
>         .allow_runtime_pm = true,
>  #endif
>         .add_host       = gl9763e_add_host,
> +       .priv_size      = sizeof(struct gli_host),
>  };
> --
> 2.36.1
>

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index d09728c37d03..891bcd38fd6d 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -95,9 +95,12 @@ 
 #define PCIE_GLI_9763E_SCR	 0x8E0
 #define   GLI_9763E_SCR_AXI_REQ	   BIT(9)
 
+#define PCIE_GLI_9763E_CFG       0x8A0
+#define   GLI_9763E_CFG_LPSN_DIS   BIT(12)
+
 #define PCIE_GLI_9763E_CFG2      0x8A4
 #define   GLI_9763E_CFG2_L1DLY     GENMASK(28, 19)
-#define   GLI_9763E_CFG2_L1DLY_MID 0x54
+#define   GLI_9763E_CFG2_L1DLY_MID 0x54		// Set L1 entry delay time to 21us
 
 #define PCIE_GLI_9763E_MMC_CTRL  0x960
 #define   GLI_9763E_HS400_SLOW     BIT(3)
@@ -144,6 +147,10 @@ 
 
 #define GLI_MAX_TUNING_LOOP 40
 
+struct gli_host {
+	bool lpm_negotiation_enabled;
+};
+
 /* Genesys Logic chipset */
 static inline void gl9750_wt_on(struct sdhci_host *host)
 {
@@ -818,6 +825,43 @@  static void sdhci_gl9763e_dumpregs(struct mmc_host *mmc)
 	sdhci_dumpregs(mmc_priv(mmc));
 }
 
+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 gl9763e_set_lpm_negotiation(struct sdhci_pci_slot *slot, bool enable)
+{
+	struct gli_host *gli_host = sdhci_pci_priv(slot);
+
+	if (gli_host->lpm_negotiation_enabled == enable)
+		return;
+
+	gli_host->lpm_negotiation_enabled = enable;
+
+	gl9763e_set_low_power_negotiation(slot, enable);
+}
+
 static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc)
 {
 	struct cqhci_host *cq_host = mmc->cqe_private;
@@ -921,6 +965,7 @@  static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
 {
 	struct pci_dev *pdev = slot->chip->pdev;
 	u32 value;
+	struct gli_host *gli_host = sdhci_pci_priv(slot);
 
 	pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
 	value &= ~GLI_9763E_VHS_REV;
@@ -941,6 +986,9 @@  static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
 	value |= FIELD_PREP(GLI_9763E_CFG2_L1DLY, GLI_9763E_CFG2_L1DLY_MID);
 	pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG2, value);
 
+	/* Default setting of LPM negotiation is enabled. */
+	gli_host->lpm_negotiation_enabled = true;
+
 	pci_read_config_dword(pdev, PCIE_GLI_9763E_CLKRXDLY, &value);
 	value &= ~GLI_9763E_HS400_RXDLY;
 	value |= FIELD_PREP(GLI_9763E_HS400_RXDLY, GLI_9763E_HS400_RXDLY_5);
@@ -959,6 +1007,9 @@  static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
 	struct sdhci_host *host = slot->host;
 	u16 clock;
 
+	/* Enable LPM negotiatiom to allow entering L1 state */
+	gl9763e_set_lpm_negotiation(slot, true);
+
 	clock = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 	clock &= ~(SDHCI_CLOCK_PLL_EN | SDHCI_CLOCK_CARD_EN);
 	sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
@@ -989,6 +1040,9 @@  static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip)
 	clock |= SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, clock, SDHCI_CLOCK_CONTROL);
 
+	/* Disable LPM negotiatiom to avoid entering L1 state. */
+	gl9763e_set_lpm_negotiation(slot, false);
+
 	return 0;
 }
 #endif
@@ -1109,4 +1163,5 @@  const struct sdhci_pci_fixes sdhci_gl9763e = {
 	.allow_runtime_pm = true,
 #endif
 	.add_host       = gl9763e_add_host,
+	.priv_size      = sizeof(struct gli_host),
 };