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

[Adrian Hunter <adrian.hunter@intel.com>]

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@intel.com> wrote:
>>
>> On 26/05/22 10:08, jasonlai.genesyslogic@gmail.com wrote:
>>> From: Jason Lai <jasonlai.genesyslogic@gmail.com>
>>>
>>> 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@gmail.com>
>>> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
>>> ---
>>>  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),
>>>  };
>>