LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Renius Chen <reniuschengl@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Ben Chuang <Ben.Chuang@genesyslogic.com.tw>
Subject: Re: [PATCH] [v2] mmc: sdhci-pci-gli: Improve Random 4K Read Performance of GL9763E
Date: Mon, 5 Jul 2021 12:02:43 +0200	[thread overview]
Message-ID: <CAPDyKFotmw-HQpZKCOD_8kThEa0_KSPnn36FNFLKRyUHYRHQjQ@mail.gmail.com> (raw)
In-Reply-To: <20210705090050.15077-1-reniuschengl@gmail.com>

On Mon, 5 Jul 2021 at 11:00, Renius Chen <reniuschengl@gmail.com> wrote:
>
> During a sequence of random 4K read operations, the performance will be
> reduced due to spending much time on entering/exiting the low power state
> between requests. We disable the low power state negotiation of GL9763E
> during a sequence of random 4K read operations to improve the performance
> and enable it again after the operations have finished.
>
> Signed-off-by: Renius Chen <reniuschengl@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 68 ++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 302a7579a9b3..5f1f332b4241 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -88,6 +88,9 @@
>  #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
> @@ -128,6 +131,11 @@
>
>  #define GLI_MAX_TUNING_LOOP 40
>
> +struct gli_host {
> +       bool start_4k_r;
> +       int continuous_4k_r;
> +};
> +
>  /* Genesys Logic chipset */
>  static inline void gl9750_wt_on(struct sdhci_host *host)
>  {
> @@ -691,6 +699,62 @@ 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_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       struct sdhci_host *host = mmc_priv(mmc);
> +       struct mmc_command *cmd;
> +       struct sdhci_pci_slot *slot = sdhci_priv(host);
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
> +
> +       cmd = mrq->cmd;
> +
> +       if (cmd && (cmd->opcode == MMC_READ_MULTIPLE_BLOCK) && (cmd->data->blocks == 8)) {
> +               gli_host->continuous_4k_r++;
> +
> +               if ((!gli_host->start_4k_r) && (gli_host->continuous_4k_r >= 3)) {
> +                       gl9763e_set_low_power_negotiation(slot, false);
> +
> +                       gli_host->start_4k_r = true;
> +               }
> +       } else {
> +               gli_host->continuous_4k_r = 0;
> +
> +               if (gli_host->start_4k_r)       {
> +                       gl9763e_set_low_power_negotiation(slot, true);
> +
> +                       gli_host->start_4k_r = false;
> +               }
> +       }

The above code is trying to figure out what kind of storage use case
that is running, based on information about the buffers. This does not
work, simply because the buffers don't give you all the information
you need to make the right decisions.

Moreover, I am sure you would try to follow up with additional changes
on top, trying to tweak the behaviour to fit another use case - and so
on. My point is, this code doesn't belong in the lowest layer drivers.

To move forward, I suggest you explore using runtime PM in combination
with dev PM qos. In this way, the driver could implement a default
behaviour, which can be tweaked from upper layer governors for
example, but also from user space (via sysfs) allowing more
flexibility and potentially support for various more use cases.

> +
> +       sdhci_request(mmc, mrq);
> +}
> +
> +
>  static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc)
>  {
>         struct cqhci_host *cq_host = mmc->cqe_private;
> @@ -848,6 +912,9 @@ static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot)
>         gli_pcie_enable_msi(slot);
>         host->mmc_host_ops.hs400_enhanced_strobe =
>                                         gl9763e_hs400_enhanced_strobe;
> +
> +       host->mmc_host_ops.request = gl9763e_request;
> +
>         gli_set_gl9763e(slot);
>         sdhci_enable_v4_mode(host);
>
> @@ -913,4 +980,5 @@ const struct sdhci_pci_fixes sdhci_gl9763e = {
>         .suspend        = sdhci_cqhci_gli_suspend,
>  #endif
>         .add_host       = gl9763e_add_host,
> +       .priv_size      = sizeof(struct gli_host),
>  };
> --

Kind regards
Uffe

  reply	other threads:[~2021-07-05 10:03 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-05  9:00 Renius Chen
2021-07-05 10:02 ` Ulf Hansson [this message]
2021-07-05 10:59   ` Renius Chen
2021-07-05 12:50     ` Ulf Hansson
2021-07-05 15:09       ` Renius Chen
2021-07-06  9:16         ` Ulf Hansson
2021-07-06  9:54           ` Renius Chen
2021-07-06 10:08             ` Ulf Hansson
2021-07-06 10:56               ` Renius Chen
2021-07-07 12:15                 ` Ulf Hansson
2021-07-07 13:49                   ` Renius Chen
2021-07-14  2:15                     ` Renius Chen
2021-07-16 10:27                       ` Adrian Hunter
2021-07-19  9:26                         ` Renius Chen
2021-08-04  6:27                           ` Adrian Hunter
2021-08-10  4:23                             ` Renius Chen
2021-08-17 10:30                               ` Renius Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPDyKFotmw-HQpZKCOD_8kThEa0_KSPnn36FNFLKRyUHYRHQjQ@mail.gmail.com \
    --to=ulf.hansson@linaro.org \
    --cc=Ben.Chuang@genesyslogic.com.tw \
    --cc=adrian.hunter@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=reniuschengl@gmail.com \
    --subject='Re: [PATCH] [v2] mmc: sdhci-pci-gli: Improve Random 4K Read Performance of GL9763E' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).