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

[Adrian Hunter <adrian.hunter@intel.com>]

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?