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

Bough Chen
2017-07-31T07:18:31Z

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: Monday, July 31, 2017 3:04 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 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?

Yes, sure.