Hi Adrian, Please see my reply below. On Fri, May 27, 2022 at 4:54 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: > > On 26/05/22 10:08, jasonlai.genesyslogic@xxxxxxxxx wrote: > > From: Jason Lai <jasonlai.genesyslogic@xxxxxxxxx> > > > > Resend this patch due to code base updated to 5.18.0-rc3. > > > > This patch is based on patch [1] and remove data transfer length checking. > > > > 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 > > and exit L1 state. > > > > Unfortunately, entering and exiting L1 state is signal handshake in > > physical layer, software knows nothiong about it. The only way to stop > > entering L1 state is to disable hardware LPM negotiation on GL9763E. > > > > To improve read performance and take battery life into account, we reject > > L1 negotiation while executing MMC_READ_MULTIPLE_BLOCK command and enable L1 > > negotiation again when receiving non-MMC_READ_MULTIPLE_BLOCK command. > > > > [1] > > https://patchwork.kernel.org/project/linux-mmc/list/?series=510801&archive > > =both > > Really needs Ulf's response, but a minor comment below. > > > > > Signed-off-by: Renius Chen <reniuschengl@xxxxxxxxx> > > Signed-off-by: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx> > > --- > > drivers/mmc/host/sdhci-pci-gli.c | 60 +++++++++++++++++++++++++++++++- > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c > > index d09728c37d03..86200b73c0b0 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,53 @@ 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) && gli_host->lpm_negotiation_enabled) { > > + gl9763e_set_low_power_negotiation(slot, false); > > + gli_host->lpm_negotiation_enabled = false; > > + } else { > > + if (gli_host->lpm_negotiation_enabled == false) { > > Is this logic right? Wouldn't it also get here with No, the logic is wrong. The original intention of my design is keeping LPM negotiation disabled from arriving of READ_MULTIPLE_BLOCK command to arriving of non-READ_MULTIPLE_BLOCK command. So The piece of code should be written as below: if (cmd && (cmd->opcode == MMC_READ_MULTIPLE_BLOCK)) { if (gli_host->lpm_negotiation_enabled) { gl9763e_set_low_power_negotiation(slot, false); gli_host->lpm_negotiation_enabled = false; } } else { if (gli_host->lpm_negotiation_enabled == false) { gl9763e_set_low_power_negotiation(slot, true); gli_host->lpm_negotiation_enabled = true; } } Am I correct? regards, Jason Lai > cmd->opcode == MMC_READ_MULTIPLE_BLOCK && > gli_host->lpm_negotiation_enabled == false > > and then you don't want the following? > > > + gl9763e_set_low_power_negotiation(slot, true); > > + gli_host->lpm_negotiation_enabled = true; > > + } > > + } > > + > > + sdhci_request(mmc, mrq); > > +} > > + > > static void sdhci_gl9763e_cqe_pre_enable(struct mmc_host *mmc) > > { > > struct cqhci_host *cq_host = mmc->cqe_private; > > @@ -1016,6 +1070,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); > > > > @@ -1109,4 +1166,5 @@ const struct sdhci_pci_fixes sdhci_gl9763e = { > > .allow_runtime_pm = true, > > #endif > > .add_host = gl9763e_add_host, > > + .priv_size = sizeof(struct gli_host), > > }; >
