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

This patch is based on patch [1] and adopt Adrian's comment.

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 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=645165

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 | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

>This patch is based on patch [1] and adopt Adrian's comment.
>
>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 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.
>

Could you describe the impact for people unfamilar with the GL9763E?
Does this essientially disable low-power mode if the controller serviced a CMD18 last?
(which will be most of the (idle) time for reasonable scenarios, right?)
Or what exactly is the LPM negotation doing?=
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

On Mon, May 30, 2022 at 5:03 PM Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> >This patch is based on patch [1] and adopt Adrian's comment.
> >
> >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 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.
> >
>
> Could you describe the impact for people unfamilar with the GL9763E?
> Does this essientially disable low-power mode if the controller serviced a CMD18 last?
> (which will be most of the (idle) time for reasonable scenarios, right?)
> Or what exactly is the LPM negotation doing?=
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
> Managing Director: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

The I/O request flow can be simplified as below:
        request received --> mmc_command --> wait command
complete(data transfer phase)
        --> request complete --> wait-for-next-request
If the time interval between 2 stages exceeds L1 entry delay time(21
us for GL9763E), PCIe
LINK layer will enter L1 state and kernel/driver cannot know when it
occurred. When PCIe
host is going to send message/command, its LINK will exit L1 state
first. GL9763E also exits
L1 state simultaneously, but it takes a little time to get back to L0
state. If we let GL9763E
enter and exit L1 state freely, only 20% of read performance remains.
Hence, we decide to
disable LPM negotiation during READ_MULTIPLE_BLOCK command.

Considering that the PCIe LINK will also enter L1 state during
wait-for-next-request stage,
LPM negotiation also needs to be disabled in this stage. That's why we
enable/disable LPM
negotiation at the point which request received. I give an example as follows：
        CMD18 --> disable LPM negotiation --> CMD18 done
        --> CMD18 --> keep LPM negotiation disabled --> CMD18 done
        --> CMD17 --> enable LPM negotiation --> CMD17 done
        --> CMD17 --> keep LPM negotiation enabled --> CMD17 done
        --> CMD18 --> disable LPM negotiation --> CMD18 done

Hope the explanation above can answer your question.

regards,
Jason Lai

On Mon, 30 May 2022 at 10:47, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
>
> This patch is based on patch [1] and adopt Adrian's comment.

Please squash $subject patch into [1], rather than making a new patch on top.

>
> 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 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.

I am not sure how the HW decides to enter L1. That said, I wonder if
it would be a better option to turn on/off the LPM from the
->runtime_suspend|resume() callbacks instead?

Kind regards
Uffe

>
> [1] https://patchwork.kernel.org/project/linux-mmc/list/?series=645165
>
> 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 | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 86200b73c0b0..13c09202da9c 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -850,24 +850,29 @@ static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>  }
>
> +static void gl9763e_set_lpm_negotiation(struct sdhci_pci_slot *slot, bool enable)
> +{
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
> +
> +       if (gli_host->lpm_negotiation_enabled == enable)
> +               return;
> +
> +       gli_host->lpm_negotiation_enabled = enable;
> +
> +       gl9763e_set_low_power_negotiation(slot, enable);
> +}
> +
>  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) {
> -                       gl9763e_set_low_power_negotiation(slot, true);
> -                       gli_host->lpm_negotiation_enabled = true;
> -               }
> -       }
> +       if (cmd && (cmd->opcode == MMC_READ_MULTIPLE_BLOCK))
> +               gl9763e_set_lpm_negotiation(slot, false);
> +       else
> +               gl9763e_set_lpm_negotiation(slot, true);
>
>         sdhci_request(mmc, mrq);
>  }
> @@ -975,6 +980,7 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>  {
>         struct pci_dev *pdev = slot->chip->pdev;
>         u32 value;
> +       struct gli_host *gli_host = sdhci_pci_priv(slot);
>
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_VHS, &value);
>         value &= ~GLI_9763E_VHS_REV;
> @@ -995,6 +1001,9 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>         value |= FIELD_PREP(GLI_9763E_CFG2_L1DLY, GLI_9763E_CFG2_L1DLY_MID);
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG2, value);
>
> +       /* Default setting of LPM negotiation is enabled. */
> +       gli_host->lpm_negotiation_enabled = true;
> +
>         pci_read_config_dword(pdev, PCIE_GLI_9763E_CLKRXDLY, &value);
>         value &= ~GLI_9763E_HS400_RXDLY;
>         value |= FIELD_PREP(GLI_9763E_HS400_RXDLY, GLI_9763E_HS400_RXDLY_5);
> --
> 2.36.1
>