On 30/05/22 06:11, Lai Jason wrote: > 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? Looks better. You might want to consider a wrapper function like: static void gl9763e_set_lpm_negotiation(struct sdhci_pci_slot *slot, bool enable) { if (gli_host->lpm_negotiation_enabled == enable) return; gli_host->lpm_negotiation_enabled = enable; gl9763e_set_low_power_negotiation(slot, enable); } > > 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), >>> }; >>
