Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210624032543.101861-1-reniuschengl@gmail.com> <916ea01b-4265-0964-9139-8f50b80ea66e@intel.com>
In-Reply-To: <916ea01b-4265-0964-9139-8f50b80ea66e@intel.com>
From:   Renius Chen <reniuschengl@gmail.com>
Date:   Thu, 1 Jul 2021 11:47:42 +0800
Message-ID: <CAJU4x8tyO9wG_b3V7V3CDs9OgV7MDQRiYxwHEN4=NXZzC=76OQ@mail.gmail.com>
Subject: Re: [PATCH] [RESEND] mmc: sdhci-pci-gli: Improve Random 4K Read
 Performance of GL9763E
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        linux-mmc <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Ben Chuang <Ben.Chuang@genesyslogic.com.tw>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Adrian Hunter <adrian.hunter@intel.com> =E6=96=BC 2021=E5=B9=B46=E6=9C=8830=
=E6=97=A5 =E9=80=B1=E4=B8=89 =E4=B8=8B=E5=8D=888:18=E5=AF=AB=E9=81=93=EF=BC=
=9A
>
> On 24/06/21 6:25 am, Renius Chen 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 sta=
te
> > between requests. We disable the low power state negotiation of GL9763E
> > during a sequence of random 4K read operations to improve the performan=
ce
> > and enable it again after the operations have finished.
> >
> > Signed-off-by: Renius Chen <reniuschengl@gmail.com>
> > ---
> >  drivers/mmc/host/sdhci-pci-gli.c | 86 ++++++++++++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-=
pci-gli.c
> > index 302a7579a9b3..0105f728ccc4 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
> > @@ -691,6 +694,86 @@ static void sdhci_gl9763e_dumpregs(struct mmc_host=
 *mmc)
> >       sdhci_dumpregs(mmc_priv(mmc));
> >  }
> >
> > +static void gl9763e_request(struct mmc_host *mmc, struct mmc_request *=
mrq)
> > +{
> > +     struct sdhci_host *host =3D mmc_priv(mmc);
> > +     struct mmc_command *cmd;
> > +     struct sdhci_pci_slot *slot =3D sdhci_priv(host);
> > +     struct pci_dev *pdev =3D slot->chip->pdev;
> > +     u32 value;
> > +     static bool start_4k_r;
> > +     static int  continuous_4k_r;
>
> Please do not use static variables.  Instead define a struct for them and
> set .priv_size in sdhci_pci_fixes e.g.
>
> struct sdhci_pci_gli {
>         static bool start_4k_r;
>         static int  continuous_4k_r;
> };
>
> const struct sdhci_pci_fixes sdhci_gl9763e =3D {
>         .quirks         =3D SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>         .probe_slot     =3D gli_probe_slot_gl9763e,
>         .ops            =3D &sdhci_gl9763e_ops,
> #ifdef CONFIG_PM_SLEEP
>         .resume         =3D sdhci_cqhci_gli_resume,
>         .suspend        =3D sdhci_cqhci_gli_suspend,
> #endif
>         .add_host       =3D gl9763e_add_host,
>         .priv_size      =3D sizeof(struct sdhci_pci_gli);
> };
>
> And then you can get a pointer to it:
>
> struct sdhci_pci_gli *sdhci_pci_gli =3D sdhci_pci_priv(slot);
>
>
> > +
> > +     cmd =3D (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23)) ? mrq->sb=
c : mrq->cmd;
>
> mrq->cmd is always the main command, so why look at mrq->sbc?

OK, I will just check if mrq->cmd is valid or not instead of looking
at mrq->sbc.
I will also modify the other parts by your comments and submit a newer vers=
ion.
Thanks for your review.

>
> > +
> > +     if (cmd->opcode =3D=3D MMC_READ_MULTIPLE_BLOCK) {
> > +             if (cmd->data->blocks =3D=3D 8) {
> > +                     continuous_4k_r++;
> > +
> > +                     if ((!start_4k_r) && (continuous_4k_r >=3D 3)) {
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_VHS, &value);
> > +                             value &=3D ~GLI_9763E_VHS_REV;
> > +                             value |=3D FIELD_PREP(GLI_9763E_VHS_REV, =
GLI_9763E_VHS_REV_W);
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_VHS, value);
> > +
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_CFG, &value);
> > +                             value |=3D GLI_9763E_CFG_LPSN_DIS;
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_CFG, value);
> > +
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_VHS, &value);
> > +                             value &=3D ~GLI_9763E_VHS_REV;
> > +                             value |=3D FIELD_PREP(GLI_9763E_VHS_REV, =
GLI_9763E_VHS_REV_R);
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_VHS, value);
> > +
> > +                             start_4k_r =3D true;
> > +                     }
> > +             } else {
> > +                     continuous_4k_r =3D 0;
> > +
> > +                     if (start_4k_r) {
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_VHS, &value);
> > +                             value &=3D ~GLI_9763E_VHS_REV;
> > +                             value |=3D FIELD_PREP(GLI_9763E_VHS_REV, =
GLI_9763E_VHS_REV_W);
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_VHS, value);
> > +
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_CFG, &value);
> > +                             value &=3D ~GLI_9763E_CFG_LPSN_DIS;
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_CFG, value);
> > +
> > +                             pci_read_config_dword(pdev, PCIE_GLI_9763=
E_VHS, &value);
> > +                             value &=3D ~GLI_9763E_VHS_REV;
> > +                             value |=3D FIELD_PREP(GLI_9763E_VHS_REV, =
GLI_9763E_VHS_REV_R);
> > +                             pci_write_config_dword(pdev, PCIE_GLI_976=
3E_VHS, value);
> > +
> > +                             start_4k_r =3D false;
> > +                     }
> > +             }
> > +     } else {
> > +             continuous_4k_r =3D 0;
> > +
> > +             if (start_4k_r) {
> > +                     pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &=
value);
> > +                     value &=3D ~GLI_9763E_VHS_REV;
> > +                     value |=3D FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763=
E_VHS_REV_W);
> > +                     pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, =
value);
> > +
> > +                     pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG, &=
value);
> > +                     value &=3D ~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 &=3D ~GLI_9763E_VHS_REV;
> > +                     value |=3D FIELD_PREP(GLI_9763E_VHS_REV, GLI_9763=
E_VHS_REV_R);
> > +                     pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, =
value);
> > +
> > +                     start_4k_r =3D false;
> > +             }
>
> The code chunk above is the same as the previous and almost
> the same as the one before that.  Please make a separate function
>
> > +     }
> > +
> > +     sdhci_request(mmc, mrq);
> > +}
> > +
> > +
> >  static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc)
> >  {
> >       struct cqhci_host *cq_host =3D mmc->cqe_private;
> > @@ -848,6 +931,9 @@ static int gli_probe_slot_gl9763e(struct sdhci_pci_=
slot *slot)
> >       gli_pcie_enable_msi(slot);
> >       host->mmc_host_ops.hs400_enhanced_strobe =3D
> >                                       gl9763e_hs400_enhanced_strobe;
> > +
> > +     host->mmc_host_ops.request =3D gl9763e_request;
> > +
> >       gli_set_gl9763e(slot);
> >       sdhci_enable_v4_mode(host);
> >
> >
>