Re: [PATCH] [RESEND] mmc: sdhci-pci-gli: Improve Random 4K Read Performance of GL9763E

From: Renius Chen
Date: Wed Jun 30 2021 - 23:47:58 EST

Adrian Hunter <adrian.hunter@xxxxxxxxx> 於 2021年6月30日 週三 下午8:18寫道：
>
> 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 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@xxxxxxxxx>
> > ---
> > 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 = mmc_priv(mmc);
> > + struct mmc_command *cmd;
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = 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 = {
> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> .probe_slot = gli_probe_slot_gl9763e,
> .ops = &sdhci_gl9763e_ops,
> #ifdef CONFIG_PM_SLEEP
> .resume = sdhci_cqhci_gli_resume,
> .suspend = sdhci_cqhci_gli_suspend,
> #endif
> .add_host = gl9763e_add_host,
> .priv_size = sizeof(struct sdhci_pci_gli);
> };
>
> And then you can get a pointer to it:
>
> struct sdhci_pci_gli *sdhci_pci_gli = sdhci_pci_priv(slot);
>
>
> > +
> > + cmd = (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23)) ? mrq->sbc : 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 version.
Thanks for your review.

>
> > +
> > + if (cmd->opcode == MMC_READ_MULTIPLE_BLOCK) {
> > + if (cmd->data->blocks == 8) {
> > + continuous_4k_r++;
> > +
> > + if ((!start_4k_r) && (continuous_4k_r >= 3)) {
> > + 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);
> > + 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);
> > +
> > + start_4k_r = true;
> > + }
> > + } else {
> > + continuous_4k_r = 0;
> > +
> > + if (start_4k_r) {
> > + 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);
> > + 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);
> > +
> > + start_4k_r = false;
> > + }
> > + }
> > + } else {
> > + continuous_4k_r = 0;
> > +
> > + if (start_4k_r) {
> > + 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);
> > + 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);
> > +
> > + start_4k_r = 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 = 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 =
> > gl9763e_hs400_enhanced_strobe;
> > +
> > + host->mmc_host_ops.request = gl9763e_request;
> > +
> > gli_set_gl9763e(slot);
> > sdhci_enable_v4_mode(host);
> >
> >
>