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

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

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().