Re: [PATCH v2] mmc: mtk-sd: reduce CIT for better performance

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Wenbin Mei <wenbin.mei@mediatek.com>,
	Ulf Hansson <ulf.hansson@linaro.org>
Cc: Chaotian Jing <chaotian.jing@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2] mmc: mtk-sd: reduce CIT for better performance
Date: Thu, 18 May 2023 11:13:22 +0200	[thread overview]
Message-ID: <0df3968e-da34-b36c-4cb4-92d66508a46a@collabora.com> (raw)
In-Reply-To: <20230510015851.11830-1-wenbin.mei@mediatek.com>

Il 10/05/23 03:58, Wenbin Mei ha scritto:
> CQHCI_SSC1 indicates to CQE the polling period to use when using periodic
> SEND_QUEUE_STATUS(CMD13) polling.
> The default value 0x1000 that corresponds to 150us, let's decrease it to

The default value 0x1000 (4096) corresponds to 4096 * 52.08uS = 231.33uS
...so the default is not 150uS.

If I'm wrong, this means that the CQCAP field is not 0, which would mean
that the expected 3uS would be wrong.

Also, since the calculation can be done dynamically, this is what we should
actually do in the driver, as this gives information to the next engineer
checking this piece of code.

Apart from this, by just writing 0x40 to the CQHCI_SSC1 register, you are
assuming that the CQCAP value requirement is fullfilled, but you cannot
assume that the bootloader has set the CQCAP's ITCFVAL and ITCFMUL fields
as you expect on all platforms: this means that implementing this takes
a little more effort.

You have two ways to implement this:
  *** First ***
  1. Read ITCFMUL and ITCFVAL, then:
     tclk_mul = itcfmul_to_mhz(ITCFMUL); /* pseudo function interprets reg value*/
     tclk = ITCFVAL * tclk_mul;

  2. Set SSC1 so that we get 3nS:
     #define CQHCI_SSC1_CIT GENMASK(15, 0)
     poll_time = cit_time_ns_to_regval(3);
     sscit = FIELD_PREP(CQHCI_SSC1_CIT, poll_time)
     cqhci_writel( ... )

  *** Second **

  1. Pre-set ITCFMUL and ITCFVAL to
     ITCFVAL = 192 (decimal)
     ITCFMUL = 2 (where 2 == 0.1MHz)

  2. Set SSC1 so that we get 3nS:
     #define CQHCI_SSC1_CIT GENMASK(15, 0)
     poll_time = cit_time_ns_to_regval(3);
     sscit = FIELD_PREP(CQHCI_SSC1_CIT, poll_time)
     cqhci_writel( ... )

I would implement the first way, as it paves the way to extend this to different
tclk values if needed in the future.

Regards,
Angelo

> 0x40 that corresponds to 3us, which can improve the performance of some
> eMMC devices.
> 
> Signed-off-by: Wenbin Mei <wenbin.mei@mediatek.com>
> ---
>   drivers/mmc/host/mtk-sd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index edade0e54a0c..ffeccddcd028 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -2453,6 +2453,7 @@ static void msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
>   static void msdc_cqe_enable(struct mmc_host *mmc)
>   {
>   	struct msdc_host *host = mmc_priv(mmc);
> +	struct cqhci_host *cq_host = mmc->cqe_private;
>   
>   	/* enable cmdq irq */
>   	writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
> @@ -2462,6 +2463,9 @@ static void msdc_cqe_enable(struct mmc_host *mmc)
>   	msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
>   	/* default read data timeout 1s */
>   	msdc_set_timeout(host, 1000000000ULL, 0);
> +
> +	/* decrease the send status command idle timer to 3us */
> +	cqhci_writel(cq_host, 0x40, CQHCI_SSC1);
>   }
>   
>   static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)