Re: [PATCH V1] mmc: sdhci-msm: Add CQHCI support for sdhci-msm

From: Veerabhadrarao Badiganti
Date: Fri Jan 10 2020 - 03:56:15 EST


On 1/2/2020 5:00 PM, Veerabhadrarao Badiganti wrote:

On 12/20/2019 7:29 PM, Adrian Hunter wrote:
On 17/12/19 2:37 pm, Veerabhadrarao Badiganti wrote:
From: Ritesh Harjani<riteshh@xxxxxxxxxxxxxx>

This adds CQHCI support for sdhci-msm platforms.

Signed-off-by: Ritesh Harjani<riteshh@xxxxxxxxxxxxxx>
Signed-off-by: Veerabhadrarao Badiganti<vbadigan@xxxxxxxxxxxxxx>

---
This patch is based on RFC patch
https://lkml.org/lkml/2017/8/30/313

Changes since RFC:
ÂÂÂÂ- Updated settings so that TDLBA won't get reset when
ÂÂÂÂÂ CQE is enabled.
ÂÂÂÂ- Removed new compatible string and moved to supports-cqe
ÂÂÂÂÂ dt flag to identify CQE support.
ÂÂÂÂ- Incorporated review comments.

Tested on: qcs404, sc7180
---
 drivers/mmc/host/sdhci-msm.c | 115 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 3d0bb5e..a4e3507 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
  #include "sdhci-pltfm.h"
+#include "cqhci.h"
  #define CORE_MCI_VERSION 0x50
 #define CORE_VERSION_MAJOR_SHIFT 28
@@ -122,6 +123,10 @@
 #define msm_host_writel(msm_host, val, host, offset) \
ÂÂÂÂÂ msm_host->var_ops->msm_writel_relaxed(val, host, offset)
 +/* CQHCI vendor specific registers */
+#define CQHCI_VENDOR_CFG1ÂÂÂ 0xA00
+#define DISABLE_RST_ON_CMDQ_ENÂÂÂ (0x3 << 13)
+
 struct sdhci_msm_offset {
ÂÂÂÂÂ u32 core_hc_mode;
ÂÂÂÂÂ u32 core_mci_data_cnt;
@@ -1567,6 +1572,109 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
ÂÂÂÂÂ __sdhci_msm_set_clock(host, clock);
 }
+/*****************************************************************************\
+ * *
+ * MSM Command Queue Engine (CQE)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *
+ * *
+\*****************************************************************************/
+
+static u32 sdhci_msm_cqe_irq(struct sdhci_host *host, u32 intmask)
+{
+ÂÂÂ int cmd_error = 0;
+ÂÂÂ int data_error = 0;
+
+ÂÂÂ if (!sdhci_cqe_irq(host, intmask, &cmd_error, &data_error))
+ÂÂÂÂÂÂÂ return intmask;
+
+ÂÂÂ cqhci_irq(host->mmc, intmask, cmd_error, data_error);
+ÂÂÂ return 0;
+}
+
+void sdhci_msm_cqe_disable(struct mmc_host *mmc, bool recovery)
+{
+ÂÂÂ struct sdhci_host *host = mmc_priv(mmc);
+ÂÂÂ unsigned long flags;
+ÂÂÂ u32 ctrl;
+
+ÂÂÂ /*
+ÂÂÂÂ * When CQE is halted, the legacy SDHCI path operates only
+ÂÂÂÂ * on 128bit descriptors in 64bit mode.
+ÂÂÂÂ */
+ÂÂÂ if (host->flags & SDHCI_USE_64_BIT_DMA)
+ÂÂÂÂÂÂÂ host->desc_sz = 16;
The adma_table_sz depends on desc_sz, so it cannot be changed here.
If you do something like below, then you can set desc_sz before calling
sdhci_setup_host()

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4540f9892ce..f1d3b70ff769 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3825,9 +3825,10 @@ int sdhci_setup_host(struct sdhci_host *host)
ÂÂÂÂÂÂÂÂÂ void *buf;
 Â if (host->flags & SDHCI_USE_64_BIT_DMA) {
+ÂÂÂÂÂÂÂÂÂÂÂ if (!host->desc_sz)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->desc_sz = SDHCI_ADMA2_64_DESC_SZ(host);
ÂÂÂÂÂÂÂÂÂÂÂÂÂ host->adma_table_sz = host->adma_table_cnt *
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SDHCI_ADMA2_64_DESC_SZ(host);
-ÂÂÂÂÂÂÂÂÂÂÂ host->desc_sz = SDHCI_ADMA2_64_DESC_SZ(host);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->desc_sz;
ÂÂÂÂÂÂÂÂÂ } else {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ host->adma_table_sz = host->adma_table_cnt *
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SDHCI_ADMA2_32_DESC_SZ;

Thanks Adrian for the suggestion. I will add this change.

But even with this change, still i will have to override 'host->desc_sz' variable since qcom sdhci controller expects/operates-on

12-byte descriptor as long was CQE is not enabled. When CQE is enabled, it operates only on 16-bype descriptors (even when CQE is halted).

If i fix "host->desc_sz" to 16 then all the data transfer commands during card initialization (till CQE is enabled) would fail.

I may have to update as below:

ÂÂÂ host->desc_sz = 16;

ÂÂÂ sdhci_add_host()Â ;

ÂÂ host->desc_sz = 12;

And then cqhci_host_ops->enable() -> host->desc_sz = 16;

Please let me know if this is fine or if you have any other suggestion to support this limitation of qcom controller w.r.t ADMA descriptors with CQE.

Hi Adrian,

Do you have any suggestions on the way to support both the descriptor sizes?

+
+ÂÂÂ spin_lock_irqsave(&host->lock, flags);
+
+ÂÂÂ /*
+ÂÂÂÂ * During CQE operation command complete bit gets latched.
+ÂÂÂÂ * So s/w should clear command complete interrupt status when CQE is
+ÂÂÂÂ * halted. Otherwise unexpected SDCHI legacy interrupt gets
+ÂÂÂÂ * triggered when CQE is halted.
+ÂÂÂÂ */
+ÂÂÂ ctrl = sdhci_readl(host, SDHCI_INT_ENABLE);
+ÂÂÂ ctrl |= SDHCI_INT_RESPONSE;
+ sdhci_writel(host, ctrl, SDHCI_INT_ENABLE);
+ÂÂÂ sdhci_writel(host, SDHCI_INT_RESPONSE, SDHCI_INT_STATUS);
+
+ÂÂÂ spin_unlock_irqrestore(&host->lock, flags);
+
+ÂÂÂ sdhci_cqe_disable(mmc, recovery);
+}
+
+static const struct cqhci_host_ops sdhci_msm_cqhci_ops = {
+ÂÂÂ .enableÂÂÂÂÂÂÂ = sdhci_cqe_enable,
+ÂÂÂ .disableÂÂÂ = sdhci_msm_cqe_disable,
+};
+
+static int sdhci_msm_cqe_add_host(struct sdhci_host *host,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct platform_device *pdev)
+{
+ÂÂÂ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ÂÂÂ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ÂÂÂ struct cqhci_host *cq_host;
+ÂÂÂ bool dma64;
+ÂÂÂ int ret;
+
+ÂÂÂ ret = sdhci_setup_host(host);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ cq_host = cqhci_pltfm_init(pdev);
+ÂÂÂ if (IS_ERR(cq_host)) {
+ÂÂÂÂÂÂÂ ret = PTR_ERR(cq_host);
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "cqhci-pltfm init: failed: %d\n", ret);
+ÂÂÂÂÂÂÂ goto cleanup;
+ÂÂÂ }
+
+ÂÂÂ msm_host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
+ÂÂÂ cq_host->ops = &sdhci_msm_cqhci_ops;
+
+ÂÂÂ dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
+
+ÂÂÂ ret = cqhci_init(cq_host, host->mmc, dma64);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "%s: CQE init: failed (%d)\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mmc_hostname(host->mmc), ret);
+ÂÂÂÂÂÂÂ goto cleanup;
+ÂÂÂ }
+
+ÂÂÂ /* Disable cqe reset due to cqe enable signal */
+ÂÂÂ cqhci_writel(cq_host, cqhci_readl(cq_host, CQHCI_VENDOR_CFG1) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ DISABLE_RST_ON_CMDQ_EN, CQHCI_VENDOR_CFG1);
+
+ÂÂÂ ret = __sdhci_add_host(host);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ goto cleanup;
+
+ÂÂÂ dev_info(&pdev->dev, "%s: CQE init: success\n",
+ÂÂÂÂÂÂÂÂÂÂÂ mmc_hostname(host->mmc));
+ÂÂÂ return ret;
+
+cleanup:
+ÂÂÂ sdhci_cleanup_host(host);
+ÂÂÂ return ret;
+}
+
 /*
ÂÂ * Platform specific register write functions. This is so that, if any
ÂÂ * register write needs to be followed up by platform specific actions,
@@ -1731,6 +1839,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
ÂÂÂÂÂ .set_uhs_signaling = sdhci_msm_set_uhs_signaling,
ÂÂÂÂÂ .write_w = sdhci_msm_writew,
ÂÂÂÂÂ .write_b = sdhci_msm_writeb,
+ÂÂÂ .irqÂÂÂ = sdhci_msm_cqe_irq,
 };
  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -1754,6 +1863,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
ÂÂÂÂÂ u8 core_major;
ÂÂÂÂÂ const struct sdhci_msm_offset *msm_offset;
ÂÂÂÂÂ const struct sdhci_msm_variant_info *var_info;
+ÂÂÂ struct device_node *node = pdev->dev.of_node;
 Â host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
ÂÂÂÂÂ if (IS_ERR(host))
@@ -1952,7 +2062,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
ÂÂÂÂÂ pm_runtime_use_autosuspend(&pdev->dev);
 Â host->mmc_host_ops.execute_tuning = sdhci_msm_execute_tuning;
-ÂÂÂ ret = sdhci_add_host(host);
+ÂÂÂ if (of_property_read_bool(node, "supports-cqe"))
+ÂÂÂÂÂÂÂ ret = sdhci_msm_cqe_add_host(host, pdev);
+ÂÂÂ else
+ÂÂÂÂÂÂÂ ret = sdhci_add_host(host);
ÂÂÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂ goto pm_runtime_disable;
ÂÂÂÂÂ sdhci_msm_set_regulator_caps(msm_host);

Thanks

Veera