lkml.org 
[lkml]   [2021]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] [RESEND] mmc: sdhci-pci-gli: Improve Random 4K Read Performance of GL9763E
    Adrian Hunter <adrian.hunter@intel.com> 於 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@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 = 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);
    > >
    > >
    >

    \
     
     \ /
      Last update: 2021-07-01 05:49    [W:3.131 / U:1.696 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site