lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67e52906-c71d-6434-a1c5-29c14d3c977e@allwinnertech.com>
Date:   Tue, 31 Jan 2023 19:01:58 +0800
From:   Michael Wu <michael@...winnertech.com>
To:     Michal Simek <michal.simek@....com>, adrian.hunter@...el.com,
        riteshh@...eaurora.org, asutoshd@...eaurora.org,
        ulf.hansson@...aro.org, chaotian.jing@...iatek.com,
        matthias.bgg@...il.com, kdasu.kdev@...il.com, alcooperx@...il.com,
        f.fainelli@...il.com, haibo.chen@....com, shawnguo@...nel.org,
        agross@...nel.org, andersson@...nel.org, michal.simek@...inx.com,
        thierry.reding@...il.com, jonathanh@...dia.com
Cc:     bcm-kernel-feedback-list@...adcom.com, kernel@...gutronix.de,
        festevam@...il.com, linux-imx@....com, konrad.dybcio@...aro.org,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        linux-tegra@...r.kernel.org
Subject: Re: [PATCH] mmc:mmc-cqhci:support interrupt coalescing

Dear Michal,
     Thanks for your advices, I'll provide the performance data soon.

On 2023/1/30 16:30, Michal Simek wrote:
> 
> If you look at commits on drivers/mmc/host/cqhci-core.c you will see 
> that subject style is
> mmc: cqhci:<space>
> 
> Also some tools are checking this style. Please fix it.
> 
> 
> 
> On 1/30/23 07:46, Michael Wu wrote:
>>
>> Support interrupt coalescing to reduce the frequency of mmc interrupts
> 
> Missing dot at the end of sentence. I expect that you are doing it for a 
> reason.
> I would guess that it increase performance but without any details.
> Should I enable it to get for example 20% better performance?
> 
> Commit message should be IMHO much bigger with more details and some 
> information about how coaleascing works would be also worth.
> 
>>
>> Signed-off-by: Michael Wu <michael@...winnertech.com>
>> ---
>>   drivers/mmc/host/cqhci-core.c      | 20 +++++++++++++++-----
>>   drivers/mmc/host/cqhci.h           |  5 ++++-
>>   drivers/mmc/host/mtk-sd.c          |  2 +-
>>   drivers/mmc/host/sdhci-brcmstb.c   |  2 +-
>>   drivers/mmc/host/sdhci-esdhc-imx.c |  2 +-
>>   drivers/mmc/host/sdhci-msm.c       |  2 +-
>>   drivers/mmc/host/sdhci-of-arasan.c |  2 +-
>>   drivers/mmc/host/sdhci-pci-core.c  |  2 +-
>>   drivers/mmc/host/sdhci-pci-gli.c   |  2 +-
>>   drivers/mmc/host/sdhci-tegra.c     |  2 +-
>>   drivers/mmc/host/sdhci_am654.c     |  2 +-
>>   11 files changed, 28 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mmc/host/cqhci-core.c 
>> b/drivers/mmc/host/cqhci-core.c
>> index b3d7d6d8d654..f9cdf9f04bfc 100644
>> --- a/drivers/mmc/host/cqhci-core.c
>> +++ b/drivers/mmc/host/cqhci-core.c
>> @@ -420,7 +420,7 @@ static void cqhci_disable(struct mmc_host *mmc)
>>   }
>>
>>   static void cqhci_prep_task_desc(struct mmc_request *mrq,
>> -                                struct cqhci_host *cq_host, int tag)
>> +                                struct cqhci_host *cq_host, int tag, 
>> int intr)
>>   {
>>          __le64 *task_desc = (__le64 __force *)get_desc(cq_host, tag);
>>          u32 req_flags = mrq->data->flags;
>> @@ -428,7 +428,7 @@ static void cqhci_prep_task_desc(struct 
>> mmc_request *mrq,
>>
>>          desc0 = CQHCI_VALID(1) |
>>                  CQHCI_END(1) |
>> -               CQHCI_INT(1) |
>> +               CQHCI_INT(intr) |
>>                  CQHCI_ACT(0x5) |
>>                  CQHCI_FORCED_PROG(!!(req_flags & MMC_DATA_FORCED_PRG)) |
>>                  CQHCI_DATA_TAG(!!(req_flags & MMC_DATA_DAT_TAG)) |
>> @@ -621,7 +621,7 @@ static int cqhci_request(struct mmc_host *mmc, 
>> struct mmc_request *mrq)
>>          }
>>
>>          if (mrq->data) {
>> -               cqhci_prep_task_desc(mrq, cq_host, tag);
>> +               cqhci_prep_task_desc(mrq, cq_host, tag, 
>> (cq_host->intr_clsc ? 0 : 1));
>>
>>                  err = cqhci_prep_tran_desc(mrq, cq_host, tag);
>>                  if (err) {
>> @@ -812,7 +812,7 @@ static void cqhci_finish_mrq(struct mmc_host *mmc, 
>> unsigned int tag)
>>   irqreturn_t cqhci_irq(struct mmc_host *mmc, u32 intmask, int cmd_error,
>>                        int data_error)
>>   {
>> -       u32 status;
>> +       u32 status, rval;
>>          unsigned long tag = 0, comp_status;
>>          struct cqhci_host *cq_host = mmc->cqe_private;
>>
>> @@ -856,6 +856,15 @@ irqreturn_t cqhci_irq(struct mmc_host *mmc, u32 
>> intmask, int cmd_error,
>>                  spin_unlock(&cq_host->lock);
>>          }
>>
>> +       if (cq_host->intr_clsc) {
>> +               rval = cqhci_readl(cq_host, CQHCI_IC);
>> +               rval |= CQHCI_IC_RESET;
>> +               cqhci_writel(cq_host, rval, CQHCI_IC);
>> +               rval = cqhci_readl(cq_host, CQHCI_IC);
>> +               rval &= (~CQHCI_IC_RESET);
>> +               cqhci_writel(cq_host, rval, CQHCI_IC);
>> +       }
>> +
>>          if (status & CQHCI_IS_TCL)
>>                  wake_up(&cq_host->wait_queue);
>>
>> @@ -1172,11 +1181,12 @@ static unsigned int cqhci_ver_minor(struct 
>> cqhci_host *cq_host)
>>   }
>>
>>   int cqhci_init(struct cqhci_host *cq_host, struct mmc_host *mmc,
>> -             bool dma64)
>> +             bool dma64, bool intr_clsc)
>>   {
>>          int err;
>>
>>          cq_host->dma64 = dma64;
>> +       cq_host->intr_clsc = intr_clsc;
>>          cq_host->mmc = mmc;
>>          cq_host->mmc->cqe_private = cq_host;
>>
>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
>> index ba9387ed90eb..acf90773c30a 100644
>> --- a/drivers/mmc/host/cqhci.h
>> +++ b/drivers/mmc/host/cqhci.h
>> @@ -227,6 +227,9 @@ struct cqhci_host {
>>
>>          /* 64 bit DMA */
>>          bool dma64;
>> +
>> +       /* interrupt coalescing*/
> 
> missing space.
> 
>> +       bool intr_clsc;
>>          int num_slots;
>>          int qcnt;
>>
>> @@ -312,7 +315,7 @@ struct platform_device;
>>
>>   irqreturn_t cqhci_irq(struct mmc_host *mmc, u32 intmask, int cmd_error,
>>                        int data_error);
>> -int cqhci_init(struct cqhci_host *cq_host, struct mmc_host *mmc, bool 
>> dma64);
>> +int cqhci_init(struct cqhci_host *cq_host, struct mmc_host *mmc, bool 
>> dma64, bool intr_clsc);
>>   struct cqhci_host *cqhci_pltfm_init(struct platform_device *pdev);
>>   int cqhci_deactivate(struct mmc_host *mmc);
>>   static inline int cqhci_suspend(struct mmc_host *mmc)
>> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
>> index edade0e54a0c..2c18f954d4b8 100644
>> --- a/drivers/mmc/host/mtk-sd.c
>> +++ b/drivers/mmc/host/mtk-sd.c
>> @@ -2796,7 +2796,7 @@ static int msdc_drv_probe(struct platform_device 
>> *pdev)
>>                  host->cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>                  host->cq_host->mmio = host->base + 0x800;
>>                  host->cq_host->ops = &msdc_cmdq_ops;
>> -               ret = cqhci_init(host->cq_host, mmc, true);
>> +               ret = cqhci_init(host->cq_host, mmc, true, false);
>>                  if (ret)
>>                          goto host_free;
>>                  mmc->max_segs = 128;
>> diff --git a/drivers/mmc/host/sdhci-brcmstb.c 
>> b/drivers/mmc/host/sdhci-brcmstb.c
>> index f2cf3d70db79..4aeaeddbbf25 100644
>> --- a/drivers/mmc/host/sdhci-brcmstb.c
>> +++ b/drivers/mmc/host/sdhci-brcmstb.c
>> @@ -231,7 +231,7 @@ static int sdhci_brcmstb_add_host(struct 
>> sdhci_host *host,
>>                  cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>          }
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret)
>>                  goto cleanup;
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c 
>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 9e73c34b6401..7aef7abe71f1 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -1712,7 +1712,7 @@ static int sdhci_esdhc_imx_probe(struct 
>> platform_device *pdev)
>>                  cq_host->mmio = host->ioaddr + ESDHC_CQHCI_ADDR_OFFSET;
>>                  cq_host->ops = &esdhc_cqhci_ops;
>>
>> -               err = cqhci_init(cq_host, host->mmc, false);
>> +               err = cqhci_init(cq_host, host->mmc, false, false);
>>                  if (err)
>>                          goto disable_ahb_clk;
>>          }
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4ac8651d0b29..b6549d1e43ec 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -2153,7 +2153,7 @@ static int sdhci_msm_cqe_add_host(struct 
>> sdhci_host *host,
>>          if (ret)
>>                  goto cleanup;
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret) {
>>                  dev_err(&pdev->dev, "%s: CQE init: failed (%d)\n",
>>                                  mmc_hostname(host->mmc), ret);
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index 89c431a34c43..811f8686532d 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -1610,7 +1610,7 @@ static int sdhci_arasan_add_host(struct 
>> sdhci_arasan_data *sdhci_arasan)
>>          if (dma64)
>>                  cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret)
>>                  goto cleanup;
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c 
>> b/drivers/mmc/host/sdhci-pci-core.c
>> index c359f867df0a..6f6cae6355a7 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -964,7 +964,7 @@ static int glk_emmc_add_host(struct sdhci_pci_slot 
>> *slot)
>>          if (dma64)
>>                  cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret)
>>                  goto cleanup;
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-gli.c 
>> b/drivers/mmc/host/sdhci-pci-gli.c
>> index 633a8ee8f8c5..6917ba339aa9 100644
>> --- a/drivers/mmc/host/sdhci-pci-gli.c
>> +++ b/drivers/mmc/host/sdhci-pci-gli.c
>> @@ -908,7 +908,7 @@ static int gl9763e_add_host(struct sdhci_pci_slot 
>> *slot)
>>          if (dma64)
>>                  cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret)
>>                  goto cleanup;
>>
>> diff --git a/drivers/mmc/host/sdhci-tegra.c 
>> b/drivers/mmc/host/sdhci-tegra.c
>> index bff084f178c9..f98a468e8f43 100644
>> --- a/drivers/mmc/host/sdhci-tegra.c
>> +++ b/drivers/mmc/host/sdhci-tegra.c
>> @@ -1620,7 +1620,7 @@ static int sdhci_tegra_add_host(struct 
>> sdhci_host *host)
>>          if (dma64)
>>                  cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>>
>> -       ret = cqhci_init(cq_host, host->mmc, dma64);
>> +       ret = cqhci_init(cq_host, host->mmc, dma64, false);
>>          if (ret)
>>                  goto cleanup;
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c 
>> b/drivers/mmc/host/sdhci_am654.c
>> index 7ef828942df3..8e7fbee70e16 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -568,7 +568,7 @@ static int sdhci_am654_cqe_add_host(struct 
>> sdhci_host *host)
>>
>>          host->mmc->caps2 |= MMC_CAP2_CQE;
>>
>> -       return cqhci_init(cq_host, host->mmc, 1);
>> +       return cqhci_init(cq_host, host->mmc, 1, false);
>>   }
>>
>>   static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
>> -- 
>> 2.29.0
>>
> 
> M

-- 
Regards,
Michael Wu

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ