Re: [PATCH V4 10/11] mmc: cqhci: support for command queue enabled host

Adrian Hunter
2017-07-31T07:10:11Z

On 31/07/17 10:03, Bough Chen wrote:
>> -----Original Message-----
>> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
>> owner@vger.kernel.org] On Behalf Of Adrian Hunter
>> Sent: Monday, July 31, 2017 2:41 PM
>> To: Bough Chen <haibo.chen@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: linux-mmc <linux-mmc@vger.kernel.org>; Alex Lemberg
>> <alex.lemberg@sandisk.com>; Mateusz Nowak <mateusz.nowak@intel.com>;
>> Yuliy Izrailov <Yuliy.Izrailov@sandisk.com>; Jaehoon Chung
>> <jh80.chung@samsung.com>; Dong Aisheng <dongas86@gmail.com>; Das
>> Asutosh <asutoshd@codeaurora.org>; Zhangfei Gao
>> <zhangfei.gao@gmail.com>; Dorfman Konstantin
>> <kdorfman@codeaurora.org>; Sahitya Tummala <stummala@codeaurora.org>;
>> Harjani Ritesh <riteshh@codeaurora.org>; Venu Byravarasu
>> <vbyravarasu@nvidia.com>; Linus Walleij <linus.walleij@linaro.org>; Shawn Lin
>> <shawn.lin@rock-chips.com>
>> Subject: Re: [PATCH V4 10/11] mmc: cqhci: support for command queue
>> enabled host
>>
>> On 24/07/17 13:21, Adrian Hunter wrote:
>>> On 24/07/17 11:52, Bough Chen wrote:
>>>> [...]
>>>>
>>>>> -----Original Message-----
>>>>> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
>>>>> Sent: Friday, July 21, 2017 5:50 PM
>>>>> To: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Cc: linux-mmc <linux-mmc@vger.kernel.org>; Bough Chen
>>>>> <haibo.chen@nxp.com>; Alex Lemberg <alex.lemberg@sandisk.com>;
>>>>> Mateusz Nowak <mateusz.nowak@intel.com>; Yuliy Izrailov
>>>>> <Yuliy.Izrailov@sandisk.com>; Jaehoon Chung
>>>>> <jh80.chung@samsung.com>; Dong Aisheng <dongas86@gmail.com>; Das
>>>>> Asutosh <asutoshd@codeaurora.org>; Zhangfei Gao
>>>>> <zhangfei.gao@gmail.com>; Dorfman Konstantin
>>>>> <kdorfman@codeaurora.org>; David Griego <david.griego@linaro.org>;
>>>>> Sahitya Tummala <stummala@codeaurora.org>; Harjani Ritesh
>>>>> <riteshh@codeaurora.org>; Venu Byravarasu <vbyravarasu@nvidia.com>;
>>>>> Linus Walleij <linus.walleij@linaro.org>; Shawn Lin
>>>>> <shawn.lin@rock-chips.com>
>>>>> Subject: [PATCH V4 10/11] mmc: cqhci: support for command queue
>>>>> enabled host
>>>>>
>>>>> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>>>>>
>>>>> This patch adds CMDQ support for command-queue compatible hosts.
>>>>>
>>>>> Command queue is added in eMMC-5.1 specification. This enables the
>>>>> controller to process upto 32 requests at a time.
>>>>>
>>>>> Adrian Hunter contributed renaming to cqhci, recovery, suspend and
>>>>> resume, cqhci_off, cqhci_wait_for_idle, and external timeout handling.
>>>>>
>>>>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>>>>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>>>> Signed-off-by: Konstantin Dorfman <kdorfman@codeaurora.org>
>>>>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>> ---
>>>>> drivers/mmc/host/Kconfig | 13 +
>>>>> drivers/mmc/host/Makefile | 1 +
>>>>> drivers/mmc/host/cqhci.c | 1146
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>> drivers/mmc/host/cqhci.h | 240 ++++++++++
>>>>> 4 files changed, 1400 insertions(+) create mode 100644
>>>>> drivers/mmc/host/cqhci.c create mode 100644
>>>>> drivers/mmc/host/cqhci.h
>>>>>
>>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>>> index 2242633550df..3a164a03f2bc 100644
>>>>> --- a/drivers/mmc/host/Kconfig
>>>>> +++ b/drivers/mmc/host/Kconfig
>>>>> @@ -834,6 +834,19 @@ config MMC_SUNXI
>>>>> This selects support for the SD/MMC Host Controller on
>>>>> Allwinner sunxi SoCs.
>>>>>
>>>>> +config MMC_CQHCI
>>>>> + tristate "Command Queue Host Controller Interface support"
>>>>> + depends on HAS_DMA
>>>>> + help
>>>>> + This selects the Command Queue Host Controller Interface (CQHCI)
>>>>> + support present in host controllers of Qualcomm Technologies, Inc
>>>>> + amongst others.
>>>>> + This controller supports eMMC devices with command queue support.
>>>>> +
>>>>> + If you have a controller with this interface, say Y or M here.
>>>>> +
>>>>> + If unsure, say N.
>>>>> +
>>>>> config MMC_TOSHIBA_PCI
>>>>> tristate "Toshiba Type A SD/MMC Card Interface Driver"
>>>>> depends on PCI
>>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>>> index
>>>>> 8c46766c000c..3ae71e006890 100644
>>>>> --- a/drivers/mmc/host/Makefile
>>>>> +++ b/drivers/mmc/host/Makefile
>>>>> @@ -88,6 +88,7 @@ obj-$(CONFIG_MMC_SDHCI_MSM) +=
>>>>> sdhci-msm.o
>>>>> obj-$(CONFIG_MMC_SDHCI_ST) += sdhci-st.o
>>>>> obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32) += sdhci-pic32.o
>>>>> obj-$(CONFIG_MMC_SDHCI_BRCMSTB) += sdhci-brcmstb.o
>>>>> +obj-$(CONFIG_MMC_CQHCI) += cqhci.o
>>>>>
>>>>> ifeq ($(CONFIG_CB710_DEBUG),y)
>>>>> CFLAGS-cb710-mmc += -DDEBUG
>>>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c new
>>>>> file mode 100644 index 000000000000..302421a26230
>>>>> --- /dev/null
>>>>> +++ b/drivers/mmc/host/cqhci.c
>>>>> @@ -0,0 +1,1146 @@
>>>>
>>>> <SNIP>
>>>>
>>>>> +
>>>>> +static int cqhci_request(struct mmc_host *mmc, struct mmc_request
>>>>> +*mrq) {
>>>>> + int err = 0;
>>>>> + u64 data = 0;
>>>>> + u64 *task_desc = NULL;
>>>>> + int tag = cqhci_tag(mrq);
>>>>> + struct cqhci_host *cq_host = mmc->cqe_private;
>>>>> + unsigned long flags;
>>>>> +
>>>>> + if (!cq_host->enabled) {
>>>>> + pr_err("%s: cqhci: not enabled\n", mmc_hostname(mmc));
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + /* First request after resume has to re-enable */
>>>>> + if (!cq_host->activated)
>>>>> + __cqhci_enable(cq_host);
>>>>> +
>>>>> + if (!mmc->cqe_on) {
>>>>> + cqhci_writel(cq_host, 0, CQHCI_CTL);
>>>>> + mmc->cqe_on = true;
>>>>> + pr_debug("%s: cqhci: CQE on\n", mmc_hostname(mmc));
>>>>> + if (cqhci_readl(cq_host, CQHCI_CTL) && CQHCI_HALT) {
>>>>> + pr_err("%s: cqhci: CQE failed to exit halt state\n",
>>>>> + mmc_hostname(mmc));
>>>>> + }
>>>>> + if (cq_host->ops->enable)
>>>>> + cq_host->ops->enable(mmc);
>>>>> + }
>>>>> +
>>>>> + if (mrq->data) {
>>>>> + task_desc = (__le64 __force *)get_desc(cq_host, tag);
>>>>> + cqhci_prep_task_desc(mrq, &data, 1);
>>>>> + *task_desc = cpu_to_le64(data);
>>>>> + err = cqhci_prep_tran_desc(mrq, cq_host, tag);
>>>>> + if (err) {
>>>>> + pr_err("%s: cqhci: failed to setup tx desc: %d\n",
>>>>> + mmc_hostname(mmc), err);
>>>>> + return err;
>>>>> + }
>>>>> + } else {
>>>>> + cqhci_prep_dcmd_desc(mmc, mrq);
>>>>> + }
>>>>> +
>>>> Hi Adrian,
>>>>
>>>> For cqhci data request, I think we need to also config
>> SDHCI_TRNS_BLK_CNT_EN and SDHCI_TRNS_DMA, just like function
>> sdhci_set_transfer_mode(): set SDHCI_TRNS_BLK_CNT_EN and
>> SDHCI_TRNS_DMA in every request.
>>>>
>>>> Currently, we do not do this for cqhci request, if support Runtime
>>>> PM, when runtime resume,
>>>> sdhci_runtime_resume_host() --> sdhci_init(host, 0) -->
>>>> sdhci_do_reset(host, SDHCI_RESET_ALL)
>>>> sdhci_reset() will do software reset for all, this will clear some SDHCI register,
>> including SDHCI_TRNS_BLK_CNT_EN and SDHCI_TRNS_DMA, then when use
>> cqhci request, error happens. I meet cqhci wait timeout error on our i.MX8.
>> After set these two bits, this issue gone.
>>>
>>> CQE anyway needs to write the transfer mode bit-4 so it is a bit
>>> strange that it doesn't write the other bits too. Nevertheless
>>> anything you need to set can be done in the ->enable() callback. For example
>> see glk_cqe_enable().
>>>
>>
>> How is it going?
>
> Yes, the ->enable() callback is a good place to set the i.mx special config, now CMDQ can works well
> On our i.MX8.

So your tested-by still applies, also to V4?