[PATCH v2 0/3] Add MCQ support for MTK platform

[PATCH v2 0/3] Add MCQ support for MTK platform

[Po-Wen Kao]

v1 -> v2
- Introduce MCQ SQ to CQ mapping vops and provide MTK implementation.
- Update export symbol patch

v1
- Separated from topic "[PATCH v4 0/5] Several UFS MCQ Code Changes".
  Here are some changes since last upload
  - Store irq in per host array
  - Symbol rename
  - Use ufshcd_mcq_poll_cqe_lock() instead of ufshcd_mcq_poll_cqe_nolock()
  - Handle invalid irq dts property
  - Remove ufshcd_disable_intr(hba, MCQ_CQ_EVENT_STATUS) in MCQ mode.
    This will become host quirk later.

Peter Wang (1):
  scsi: ufs: core: Introduce mcq ops to config cqid

Po-Wen Kao (2):
  scsi: ufs: core: Export symbols for MTK driver module
  scsi: ufs: ufs-mediatek: Add MCQ support for MTK platform

 drivers/ufs/core/ufs-mcq.c      |   6 +-
 drivers/ufs/core/ufshcd-priv.h  |  10 +-
 drivers/ufs/core/ufshcd.c       |  11 ++
 drivers/ufs/host/ufs-mediatek.c | 188 +++++++++++++++++++++++++++++++-
 drivers/ufs/host/ufs-mediatek.h |  33 ++++++
 include/ufs/ufshcd.h            |   8 ++
 6 files changed, 251 insertions(+), 5 deletions(-)

--
2.18.0

[PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Po-Wen Kao]

From: Peter Wang <peter.wang@mediatek.com>

MCQ sq/cq mapping is not just one for one, could many for one.
This patch allow host driver to change the mapping, assign cqid
for each hw queue.

Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufs-mcq.c     |  2 +-
 drivers/ufs/core/ufshcd-priv.h |  8 ++++++++
 drivers/ufs/core/ufshcd.c      | 11 +++++++++++
 include/ufs/ufshcd.h           |  3 +++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 51b3c6ae781d..1ba9c395c6b0 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -368,7 +368,7 @@ void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba)
 		 * Submission Queue Attribute
 		 */
 		ufsmcq_writel(hba, (1 << QUEUE_EN_OFFSET) | qsize |
-			      (i << QUEUE_ID_OFFSET),
+			      (hwq->cqid << QUEUE_ID_OFFSET),
 			      MCQ_CFG_n(REG_SQATTR, i));
 	}
 }
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index d53b93c21a0c..2de068b96c71 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -287,6 +287,14 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+static inline int ufshcd_mcq_vops_config_cqid(struct ufs_hba *hba)
+{
+	if (hba->vops && hba->vops->config_cqid)
+		return hba->vops->config_cqid(hba);
+
+	return -EOPNOTSUPP;
+}
+
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4ec8dacb447c..fad9ff4469b0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8488,11 +8488,22 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba)
 static void ufshcd_config_mcq(struct ufs_hba *hba)
 {
 	int ret;
+	struct ufs_hw_queue *hwq;
+	int i;
 
 	ret = ufshcd_mcq_vops_config_esi(hba);
 	dev_info(hba->dev, "ESI %sconfigured\n", ret ? "is not " : "");
 
 	ufshcd_enable_intr(hba, UFSHCD_ENABLE_MCQ_INTRS);
+
+	ret = ufshcd_mcq_vops_config_cqid(hba);
+	if (ret) {
+		for (i = 0; i < hba->nr_hw_queues; i++) {
+			hwq = &hba->uhq[i];
+			hwq->cqid = i;
+		}
+	}
+
 	ufshcd_mcq_make_queues_operational(hba);
 	ufshcd_mcq_config_mac(hba, hba->nutrs);
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index df1d04f7a542..d5b16f968d7f 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -307,6 +307,7 @@ struct ufs_pwr_mode_info {
  * @op_runtime_config: called to config Operation and runtime regs Pointers
  * @get_outstanding_cqs: called to get outstanding completion queues
  * @config_esi: called to config Event Specific Interrupt
+ * @config_cqid: called to config cqid for each sq
  */
 struct ufs_hba_variant_ops {
 	const char *name;
@@ -352,6 +353,7 @@ struct ufs_hba_variant_ops {
 	int	(*get_outstanding_cqs)(struct ufs_hba *hba,
 				       unsigned long *ocqs);
 	int	(*config_esi)(struct ufs_hba *hba);
+	int	(*config_cqid)(struct ufs_hba *hba);
 };
 
 /* clock gating state  */
@@ -1100,6 +1102,7 @@ struct ufs_hw_queue {
 	dma_addr_t cqe_dma_addr;
 	u32 max_entries;
 	u32 id;
+	u32 cqid;
 	u32 sq_tail_slot;
 	spinlock_t sq_lock;
 	u32 cq_tail_slot;
-- 
2.18.0

[PATCH v2 2/3] scsi: ufs: core: Export symbols for MTK driver module

[Po-Wen Kao]

Export
- ufshcd_mcq_config_mac
- ufshcd_mcq_make_queues_operational
- ufshcd_mcq_read_cqis
- ufshcd_mcq_poll_cqe_lock

Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
 drivers/ufs/core/ufs-mcq.c     | 4 ++++
 drivers/ufs/core/ufshcd-priv.h | 2 --
 include/ufs/ufshcd.h           | 5 +++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 1ba9c395c6b0..1845c26c6958 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -92,6 +92,7 @@ void ufshcd_mcq_config_mac(struct ufs_hba *hba, u32 max_active_cmds)
 	val |= FIELD_PREP(MCQ_CFG_MAC_MASK, max_active_cmds);
 	ufshcd_writel(hba, val, REG_UFS_MCQ_CFG);
 }
+EXPORT_SYMBOL_GPL(ufshcd_mcq_config_mac);
 
 /**
  * ufshcd_mcq_req_to_hwq - find the hardware queue on which the
@@ -242,6 +243,7 @@ u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i)
 {
 	return readl(mcq_opr_base(hba, OPR_CQIS, i) + REG_CQIS);
 }
+EXPORT_SYMBOL_GPL(ufshcd_mcq_read_cqis);
 
 void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i)
 {
@@ -308,6 +310,7 @@ unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 
 	return completed_reqs;
 }
+EXPORT_SYMBOL_GPL(ufshcd_mcq_poll_cqe_lock);
 
 void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba)
 {
@@ -372,6 +375,7 @@ void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba)
 			      MCQ_CFG_n(REG_SQATTR, i));
 	}
 }
+EXPORT_SYMBOL_GPL(ufshcd_mcq_make_queues_operational);
 
 void ufshcd_mcq_enable_esi(struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 2de068b96c71..5b9c30284a30 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -66,8 +66,6 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 int ufshcd_mcq_init(struct ufs_hba *hba);
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
 int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
-void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba);
-void ufshcd_mcq_config_mac(struct ufs_hba *hba, u32 max_active_cmds);
 void ufshcd_mcq_select_mcq_mode(struct ufs_hba *hba);
 u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
 void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d5b16f968d7f..ad9d42a8fbfc 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1242,9 +1242,14 @@ void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
 void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val);
 void ufshcd_hba_stop(struct ufs_hba *hba);
 void ufshcd_schedule_eh_work(struct ufs_hba *hba);
+void ufshcd_mcq_config_mac(struct ufs_hba *hba, u32 max_active_cmds);
+u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
 void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
 unsigned long ufshcd_mcq_poll_cqe_nolock(struct ufs_hba *hba,
 					 struct ufs_hw_queue *hwq);
+unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
+					 struct ufs_hw_queue *hwq);
+void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba);
 void ufshcd_mcq_enable_esi(struct ufs_hba *hba);
 void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg);
 
-- 
2.18.0

[PATCH v2 3/3] scsi: ufs: ufs-mediatek: Add MCQ support for MTK platform

[Po-Wen Kao]

Changes
- Implement vops and setup irq
- Fix pm flow under mcq mode

Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/host/ufs-mediatek.c | 188 +++++++++++++++++++++++++++++++-
 drivers/ufs/host/ufs-mediatek.h |  33 ++++++
 2 files changed, 219 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index a054810e321d..c0bc9ebf0830 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -27,8 +27,14 @@
 #include <ufs/unipro.h>
 #include "ufs-mediatek.h"

+static int  ufs_mtk_config_mcq(struct ufs_hba *hba, bool irq);
+
 #define CREATE_TRACE_POINTS
 #include "ufs-mediatek-trace.h"
+#undef CREATE_TRACE_POINTS
+
+#define MAX_SUPP_MAC 64
+#define MCQ_QUEUE_OFFSET(c) ((((c) >> 16) & 0xFF) * 0x200)

 static const struct ufs_dev_quirk ufs_mtk_dev_fixups[] = {
 	{ .wmanufacturerid = UFS_ANY_VENDOR,
@@ -840,6 +846,37 @@ static void ufs_mtk_vreg_fix_vccqx(struct ufs_hba *hba)
 	}
 }

+static void ufs_mtk_init_mcq_irq(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+	struct platform_device *pdev;
+	int i;
+	int irq;
+
+	host->mcq_nr_intr = UFSHCD_MAX_Q_NR;
+	pdev = container_of(hba->dev, struct platform_device, dev);
+
+	/* invalidate irq info */
+	for (i = 0; i < host->mcq_nr_intr; i++)
+		host->mcq_intr_info[i].irq = MTK_MCQ_INVALID_IRQ;
+
+	for (i = 0; i < host->mcq_nr_intr; i++) {
+		/* irq index 0 is legacy irq, sq/cq irq start from index 1 */
+		irq = platform_get_irq(pdev, i + 1);
+		if (irq < 0) {
+			dev_err(hba->dev, "get platform mcq irq fail: %d\n", i);
+			goto failed;
+		}
+		host->mcq_intr_info[i].hba = hba;
+		host->mcq_intr_info[i].irq = irq;
+		dev_info(hba->dev, "get platform mcq irq: %d, %d\n", i, irq);
+	}
+
+	return;
+failed:
+	host->mcq_nr_intr = 0;
+}
+
 /**
  * ufs_mtk_init - find other essential mmio bases
  * @hba: host controller instance
@@ -876,6 +913,8 @@ static int ufs_mtk_init(struct ufs_hba *hba)
 	/* Initialize host capability */
 	ufs_mtk_init_host_caps(hba);

+	ufs_mtk_init_mcq_irq(hba);
+
 	err = ufs_mtk_bind_mphy(hba);
 	if (err)
 		goto out_variant_clear;
@@ -1171,7 +1210,16 @@ static int ufs_mtk_link_set_hpm(struct ufs_hba *hba)
 	else
 		return err;

-	err = ufshcd_make_hba_operational(hba);
+	if (!hba->mcq_enabled) {
+		err = ufshcd_make_hba_operational(hba);
+	} else {
+		ufs_mtk_config_mcq(hba, false);
+		ufshcd_mcq_make_queues_operational(hba);
+		ufshcd_mcq_config_mac(hba, hba->nutrs);
+		ufshcd_writel(hba, ufshcd_readl(hba, REG_UFS_MEM_CFG) | 0x1,
+			      REG_UFS_MEM_CFG);
+	}
+
 	if (err)
 		return err;

@@ -1495,6 +1543,136 @@ static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 	return 0;
 }

+static int ufs_mtk_get_hba_mac(struct ufs_hba *hba)
+{
+	return MAX_SUPP_MAC;
+}
+
+static int ufs_mtk_op_runtime_config(struct ufs_hba *hba)
+{
+	struct ufshcd_mcq_opr_info_t *opr;
+	int i;
+
+	for (i = 0; i < OPR_MAX; i++) {
+		opr = &hba->mcq_opr[i];
+		opr->stride = REG_UFS_MCQ_STRIDE;
+	}
+
+	hba->mcq_opr[OPR_SQD].offset = REG_UFS_MTK_SQD;
+	hba->mcq_opr[OPR_SQIS].offset = REG_UFS_MTK_SQIS;
+	hba->mcq_opr[OPR_CQD].offset = REG_UFS_MTK_CQD;
+	hba->mcq_opr[OPR_CQIS].offset = REG_UFS_MTK_CQIS;
+
+	hba->mcq_opr[OPR_SQD].base = hba->mmio_base + REG_UFS_MTK_SQD;
+	hba->mcq_opr[OPR_SQIS].base = hba->mmio_base + REG_UFS_MTK_SQIS;
+	hba->mcq_opr[OPR_CQD].base = hba->mmio_base + REG_UFS_MTK_CQD;
+	hba->mcq_opr[OPR_CQIS].base = hba->mmio_base + REG_UFS_MTK_CQIS;
+
+	return 0;
+}
+
+static int ufs_mtk_mcq_config_resource(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+
+	/* fail mcq initialization if interrupt is not filled properly */
+	if (!host->mcq_nr_intr) {
+		dev_info(hba->dev, "IRQs not ready. MCQ disabled.");
+		return -EINVAL;
+	}
+
+	hba->mcq_base = hba->mmio_base + MCQ_QUEUE_OFFSET(hba->mcq_capabilities);
+	return 0;
+}
+
+static irqreturn_t ufs_mtk_mcq_intr(int irq, void *__intr_info)
+{
+	struct ufs_mtk_mcq_intr_info *mcq_intr_info = __intr_info;
+	struct ufs_hba *hba = mcq_intr_info->hba;
+	struct ufs_hw_queue *hwq;
+	u32 events;
+	int i = mcq_intr_info->qid;
+
+	hwq = &hba->uhq[i];
+
+	events = ufshcd_mcq_read_cqis(hba, i);
+	if (events)
+		ufshcd_mcq_write_cqis(hba, events, i);
+
+	if (events & UFSHCD_MCQ_CQIS_TAIL_ENT_PUSH_STS)
+		ufshcd_mcq_poll_cqe_lock(hba, hwq);
+
+	return IRQ_HANDLED;
+}
+
+static int ufs_mtk_config_mcq_irq(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+	u32 irq, i;
+	int ret;
+
+	for (i = 0; i < host->mcq_nr_intr; i++) {
+		irq = host->mcq_intr_info[i].irq;
+		if (irq == MTK_MCQ_INVALID_IRQ) {
+			dev_err(hba->dev, "invalid irq. %d\n", i);
+			return -ENOPARAM;
+		}
+
+		host->mcq_intr_info[i].qid = i;
+		ret = devm_request_irq(hba->dev, irq, ufs_mtk_mcq_intr, 0, UFSHCD,
+				       &host->mcq_intr_info[i]);
+
+		dev_info(hba->dev, "request irq %d intr %s\n", irq, ret ? "failed" : "");
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ufs_mtk_config_mcq(struct ufs_hba *hba, bool irq)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+	int ret = 0;
+
+	if (!host->mcq_set_intr) {
+		/* Disable irq option register */
+		ufshcd_rmwl(hba, MCQ_INTR_EN_MSK, 0, REG_UFS_MMIO_OPT_CTRL_0);
+
+		if (irq)
+			ret = ufs_mtk_config_mcq_irq(hba);
+
+		if (ret)
+			return ret;
+
+		host->mcq_set_intr = true;
+	}
+
+	ufshcd_rmwl(hba, MCQ_AH8, MCQ_AH8, REG_UFS_MMIO_OPT_CTRL_0);
+	ufshcd_rmwl(hba, MCQ_INTR_EN_MSK, MCQ_MULTI_INTR_EN, REG_UFS_MMIO_OPT_CTRL_0);
+
+	return 0;
+}
+
+static int ufs_mtk_config_esi(struct ufs_hba *hba)
+{
+	return ufs_mtk_config_mcq(hba, true);
+}
+
+static int ufs_mtk_config_cqid(struct ufs_hba *hba)
+{
+	struct ufs_hw_queue *hwq;
+	int i;
+
+	for (i = 0; i < hba->nr_hw_queues; i++) {
+		hwq = &hba->uhq[i];
+		hwq->cqid = 3;
+	}
+
+	return 0;
+}
+
 /*
  * struct ufs_hba_mtk_vops - UFS MTK specific variant operations
  *
@@ -1518,6 +1696,12 @@ static const struct ufs_hba_variant_ops ufs_hba_mtk_vops = {
 	.event_notify        = ufs_mtk_event_notify,
 	.config_scaling_param = ufs_mtk_config_scaling_param,
 	.clk_scale_notify    = ufs_mtk_clk_scale_notify,
+	/* mcq vops */
+	.get_hba_mac         = ufs_mtk_get_hba_mac,
+	.op_runtime_config   = ufs_mtk_op_runtime_config,
+	.mcq_config_resource = ufs_mtk_mcq_config_resource,
+	.config_esi          = ufs_mtk_config_esi,
+	.config_cqid         = ufs_mtk_config_cqid,
 };

 /**
@@ -1564,7 +1748,7 @@ static int ufs_mtk_probe(struct platform_device *pdev)

 out:
 	if (err)
-		dev_info(dev, "probe failed %d\n", err);
+		dev_err(dev, "probe failed %d\n", err);

 	of_node_put(reset_node);
 	return err;
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index 2fc6d7b87694..6e41550e1830 100644
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -10,11 +10,27 @@
 #include <linux/pm_qos.h>
 #include <linux/soc/mediatek/mtk_sip_svc.h>

+/*
+ * MCQ define and struct
+ */
+#define UFSHCD_MAX_Q_NR 8
+#define MTK_MCQ_INVALID_IRQ	0xFFFF
+
+/* REG_UFS_MMIO_OPT_CTRL_0 160h */
+#define EHS_EN                  0x1
+#define PFM_IMPV                0x2
+#define MCQ_MULTI_INTR_EN       0x4
+#define MCQ_CMB_INTR_EN         0x8
+#define MCQ_AH8                 0x10
+
+#define MCQ_INTR_EN_MSK         (MCQ_MULTI_INTR_EN | MCQ_CMB_INTR_EN)
+
 /*
  * Vendor specific UFSHCI Registers
  */
 #define REG_UFS_XOUFS_CTRL          0x140
 #define REG_UFS_REFCLK_CTRL         0x144
+#define REG_UFS_MMIO_OPT_CTRL_0     0x160
 #define REG_UFS_EXTREG              0x2100
 #define REG_UFS_MPHYCTRL            0x2200
 #define REG_UFS_MTK_IP_VER          0x2240
@@ -26,6 +42,13 @@
 #define REG_UFS_DEBUG_SEL_B2        0x22D8
 #define REG_UFS_DEBUG_SEL_B3        0x22DC

+#define REG_UFS_MTK_SQD             0x2800
+#define REG_UFS_MTK_SQIS            0x2814
+#define REG_UFS_MTK_CQD             0x281C
+#define REG_UFS_MTK_CQIS            0x2824
+
+#define REG_UFS_MCQ_STRIDE          0x30
+
 /*
  * Ref-clk control
  *
@@ -136,6 +159,12 @@ struct ufs_mtk_hw_ver {
 	u8 major;
 };

+struct ufs_mtk_mcq_intr_info {
+	struct ufs_hba *hba;
+	u32 irq;
+	u8 qid;
+};
+
 struct ufs_mtk_host {
 	struct phy *mphy;
 	struct pm_qos_request pm_qos_req;
@@ -155,6 +184,10 @@ struct ufs_mtk_host {
 	u16 ref_clk_ungating_wait_us;
 	u16 ref_clk_gating_wait_us;
 	u32 ip_ver;
+
+	bool mcq_set_intr;
+	int mcq_nr_intr;
+	struct ufs_mtk_mcq_intr_info mcq_intr_info[UFSHCD_MAX_Q_NR];
 };

 /*
--
2.18.0

Re: [PATCH v2 0/3] Add MCQ support for MTK platform

[Stanley Chu]

Hi Po-Wen,

On Tue, May 30, 2023 at 10:44 AM Po-Wen Kao <powen.kao@mediatek.com> wrote:
>
> v1 -> v2
> - Introduce MCQ SQ to CQ mapping vops and provide MTK implementation.
> - Update export symbol patch
>
> v1
> - Separated from topic "[PATCH v4 0/5] Several UFS MCQ Code Changes".
>   Here are some changes since last upload
>   - Store irq in per host array
>   - Symbol rename
>   - Use ufshcd_mcq_poll_cqe_lock() instead of ufshcd_mcq_poll_cqe_nolock()
>   - Handle invalid irq dts property
>   - Remove ufshcd_disable_intr(hba, MCQ_CQ_EVENT_STATUS) in MCQ mode.
>     This will become host quirk later.
>
> Peter Wang (1):
>   scsi: ufs: core: Introduce mcq ops to config cqid
>
> Po-Wen Kao (2):
>   scsi: ufs: core: Export symbols for MTK driver module
>   scsi: ufs: ufs-mediatek: Add MCQ support for MTK platform
>
>  drivers/ufs/core/ufs-mcq.c      |   6 +-
>  drivers/ufs/core/ufshcd-priv.h  |  10 +-
>  drivers/ufs/core/ufshcd.c       |  11 ++
>  drivers/ufs/host/ufs-mediatek.c | 188 +++++++++++++++++++++++++++++++-
>  drivers/ufs/host/ufs-mediatek.h |  33 ++++++
>  include/ufs/ufshcd.h            |   8 ++
>  6 files changed, 251 insertions(+), 5 deletions(-)
>
> --
> 2.18.0
>

For this series, feel free to add

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[AngeloGioacchino Del Regno]

Il 30/05/23 04:32, Po-Wen Kao ha scritto:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> MCQ sq/cq mapping is not just one for one, could many for one.
> This patch allow host driver to change the mapping, assign cqid
> for each hw queue.
> 
> Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>   drivers/ufs/core/ufs-mcq.c     |  2 +-
>   drivers/ufs/core/ufshcd-priv.h |  8 ++++++++
>   drivers/ufs/core/ufshcd.c      | 11 +++++++++++
>   include/ufs/ufshcd.h           |  3 +++
>   4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 51b3c6ae781d..1ba9c395c6b0 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -368,7 +368,7 @@ void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba)
>   		 * Submission Queue Attribute
>   		 */
>   		ufsmcq_writel(hba, (1 << QUEUE_EN_OFFSET) | qsize |
> -			      (i << QUEUE_ID_OFFSET),
> +			      (hwq->cqid << QUEUE_ID_OFFSET),
>   			      MCQ_CFG_n(REG_SQATTR, i));
>   	}
>   }
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index d53b93c21a0c..2de068b96c71 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -287,6 +287,14 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
>   	return -EOPNOTSUPP;
>   }
>   
> +static inline int ufshcd_mcq_vops_config_cqid(struct ufs_hba *hba)
> +{
> +	if (hba->vops && hba->vops->config_cqid)
> +		return hba->vops->config_cqid(hba);
> +
> +	return -EOPNOTSUPP;
> +}
> +
>   extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>   
>   /**
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 4ec8dacb447c..fad9ff4469b0 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8488,11 +8488,22 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba)
>   static void ufshcd_config_mcq(struct ufs_hba *hba)
>   {
>   	int ret;
> +	struct ufs_hw_queue *hwq;
> +	int i;
>   
>   	ret = ufshcd_mcq_vops_config_esi(hba);
>   	dev_info(hba->dev, "ESI %sconfigured\n", ret ? "is not " : "");
>   
>   	ufshcd_enable_intr(hba, UFSHCD_ENABLE_MCQ_INTRS);
> +
> +	ret = ufshcd_mcq_vops_config_cqid(hba);
> +	if (ret) {

If your return value here is not -EOPNOTSUPP you may want to perform some
different action... and perhaps ufshcd_config_mcq() should be changed to
return a failure.
Should also be trivial to do so, since this function is called 3 times in
total, and only in ufshcd_device_init(), which is already returning int.

So, I would say....

static int ufshcd_config_mcq(struct ufs_hba *hba)
{

..... code .....

	ret = ufshcd_mcq_vops_config_cqid(hba);
	if (ret) {
		if (ret != -EOPNOTSUPP)
			return ret;

		/* No special configuration, go for 1:1 mapping */
		for (i = 0; ....)
			....
	}

Regards,
Angelo

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Powen Kao (高伯文)]

On Tue, 2023-05-30 at 09:20 +0200, AngeloGioacchino Del Regno wrote:
> 
> If your return value here is not -EOPNOTSUPP you may want to perform
> some
> different action... and perhaps ufshcd_config_mcq() should be changed
> to
> return a failure.
> Should also be trivial to do so, since this function is called 3
> times in
> total, and only in ufshcd_device_init(), which is already returning
> int.
> 
> So, I would say....
> 
> static int ufshcd_config_mcq(struct ufs_hba *hba)
> {
> 
> ..... code .....
> 
> ret = ufshcd_mcq_vops_config_cqid(hba);
> if (ret) {
> if (ret != -EOPNOTSUPP)
> return ret;
> 
> /* No special configuration, go for 1:1 mapping */
> for (i = 0; ....)
> ....
> }
> 
> Regards,
> Angelo
> 
> 

Hi Angelo,

That's a good point. 
Can we update this later in another patch to take care of
ufshcd_config_mcq()'s return value since this patch focus on
introducing new vops?

Best,
Po-Wen

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Bart Van Assche]

On 5/29/23 19:32, Po-Wen Kao wrote:
> MCQ sq/cq mapping is not just one for one, could many for one.
> This patch allow host driver to change the mapping, assign cqid
> for each hw queue.

What use case do you have in mind for associating multiple submission 
queues with a single completion queue?

No matter what the use case is, I think that which submission queues are 
associated with a completion queue is independent of the host driver and 
hence that such logic should exist in the UFS core instead of in a host 
driver.

Bart.

Re: [PATCH v2 3/3] scsi: ufs: ufs-mediatek: Add MCQ support for MTK platform

[Bart Van Assche]

On 5/29/23 19:32, Po-Wen Kao wrote:
> Changes
> - Implement vops and setup irq
> - Fix pm flow under mcq mode

Please use the regular style for patch descriptions and use full 
sentences instead of a bullet list.

Thanks,

Bart.

Re: [PATCH v2 2/3] scsi: ufs: core: Export symbols for MTK driver module

[Bart Van Assche]

On 5/29/23 19:32, Po-Wen Kao wrote:
> Export
> - ufshcd_mcq_config_mac
> - ufshcd_mcq_make_queues_operational
> - ufshcd_mcq_read_cqis
> - ufshcd_mcq_poll_cqe_lock

Please use the regular style for patch descriptions and use full 
sentences instead of a bullet list.

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Stanley Chu]

On Wed, May 31, 2023 at 7:54 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 5/29/23 19:32, Po-Wen Kao wrote:
> > MCQ sq/cq mapping is not just one for one, could many for one.
> > This patch allow host driver to change the mapping, assign cqid
> > for each hw queue.
>
> What use case do you have in mind for associating multiple submission
> queues with a single completion queue?
>

According to the specification or the original concept of MCQ, the
bindings between SQ and CQ can not be fixed to a single style.
In addition, some benchmark data shows that the performance can be
improved by using fewer CQs to aggregate the interrupt handling of
completion requests.
Therefore, we would like to introduce a vop to allow the host to
configure it accordingly.

Stanley

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Bart Van Assche]

On 5/30/23 18:54, Stanley Chu wrote:
> In addition, some benchmark data shows that the performance can be
> improved by using fewer CQs to aggregate the interrupt handling of
> completion requests.

What has been measured? IOPS only or both IOPS and latency?

How big is the difference? A few percent or more?

For which number of SQs and which number of CQs has performance data 
been measured?

Would the following work instead of introducing a new vop?
- Introduce a new capability flag, e.g. UFSHCD_CAP_SINGLE_CQ.
- Set that flag from inside ufs_mtk_init().
- Modify the UFS core driver such that the number of completion queues
   depends on the UFSHCD_CAP_SINGLE_CQ flag.

> Therefore, we would like to introduce a vop to allow the host to
> configure it accordingly.

We do not accept new vops upstream without a user. Where is the 
implementation of the new .config_cqid() callback?

Thanks,

Bart.

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Powen Kao (高伯文)]

On Wed, 2023-05-31 at 05:48 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 5/30/23 18:54, Stanley Chu wrote:
> > In addition, some benchmark data shows that the performance can be
> > improved by using fewer CQs to aggregate the interrupt handling of
> > completion requests.
> 
> What has been measured? IOPS only or both IOPS and latency?
> 
> How big is the difference? A few percent or more?
> For which number of SQs and which number of CQs has performance data 
> been measured?
> 
Comparing 8-8 to 8-1 mapping, it shows few percents improvement.

> 
> Would the following work instead of introducing a new vop?
> - Introduce a new capability flag, e.g. UFSHCD_CAP_SINGLE_CQ.
> - Set that flag from inside ufs_mtk_init().
> - Modify the UFS core driver such that the number of completion
> queues
>    depends on the UFSHCD_CAP_SINGLE_CQ flag.
> 
According to spec, driver is free to assign any SQ to CQ mapping. I am
not sure if it's ideal to constrain mapping to specific kind.

> > Therefore, we would like to introduce a vop to allow the host to
> > configure it accordingly.
> 
> We do not accept new vops upstream without a user. Where is the 
> implementation of the new .config_cqid() callback?
> 

Yes, please refer to
"[PATCH v2 3/3] scsi: ufs: ufs-mediatek: Add MCQ support for MTK
platform"

+static int ufs_mtk_config_cqid(struct ufs_hba *hba)
+{
+       struct ufs_hw_queue *hwq;
+       int i;
+
+       for (i = 0; i < hba->nr_hw_queues; i++) {
+               hwq = &hba->uhq[i];
+               hwq->cqid = 3;
+       }
+
+       return 0;
+}


Po-Wen

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Bart Van Assche]

On 6/1/23 03:25, Powen Kao (高伯文) wrote:
> According to spec, driver is free to assign any SQ to CQ mapping. I am
> not sure if it's ideal to constrain mapping to specific kind.

As has been made clear several times recently during discussions around
LSF/MM topics, implementing all features of a standard is *not* one of the
goals of the Linux kernel. Whether a feature is defined in the NVMe, a SCSI
or the UFS standard, we (Linux kernel community) only support those features
that we consider useful and that can be implemented with a reasonable effort.
An example of a feature that probably will never be supported by the Linux
kernel is the "domains and realms" functionality from ZBC-2.

> Yes, please refer to
> "[PATCH v2 3/3] scsi: ufs: ufs-mediatek: Add MCQ support for MTK
> platform"
> 
> +static int ufs_mtk_config_cqid(struct ufs_hba *hba)
> +{
> +       struct ufs_hw_queue *hwq;
> +       int i;
> +
> +       for (i = 0; i < hba->nr_hw_queues; i++) {
> +               hwq = &hba->uhq[i];
> +               hwq->cqid = 3;
> +       }
> +
> +       return 0;
> +}

Thanks, I had overlooked this. Do you agree that the above shows that the
flag I proposed in my previous email (UFSHCD_CAP_SINGLE_CQ) is sufficient
to support the MediaTek use case? I want to keep the SQ-CQ association code
in the UFS driver core because the next step will probably to switch from
one CQ per SQ to one CQ per CPU core for UFS controllers that support
multiple completion interrupts.

Thanks,

Bart.

Re: [PATCH v2 1/3] scsi: ufs: core: Introduce mcq ops to config cqid

[Bart Van Assche]

On 6/1/23 07:41, Peter Wang (王信友) wrote:
> On Thu, 2023-06-01 at 07:23 -0700, Bart Van Assche wrote:
>> Thanks, I had overlooked this. Do you agree that the above shows that the
>> flag I proposed in my previous email (UFSHCD_CAP_SINGLE_CQ) is sufficient
>> to support the MediaTek use case? I want to keep the SQ-CQ association code
>> in the UFS driver core because the next step will probably to switch from
>> one CQ per SQ to one CQ per CPU core for UFS controllers that support
>> multiple completion interrupts.
>
> If the UFS device speed is geting higher and higher, one CQ may not sufficient.
> 
> So, UFSHCD_CAP_SINGLE_CQ is not flexible for us beacuse we may want to map to two CQs.

Hi Peter,

Let's take a step back. The MediaTek UFSHCI 4.0 host controller only 
supports a single completion interrupt. A significant disadvantage of 
the single completion interrupt approach is that all completion 
interrupts are processed by the same CPU core. This is known to cause 
problems on Android. If sufficient time is spent in an interrupt 
handler, threads that run on the same CPU core as the interrupt handler 
may get scheduled too late. This can result in e.g. audio glitches 
noticeable by humans. Hardware designers told me that the area occupied 
by a single interrupt line is small. So I think it is fair to say that 
the (nonstandard!) approach of only supporting a single completion 
interrupt in an UFSHCI 4.0 controller is not a good choice.

The UFS driver already supports multiple hardware queue types 
(HCTX_TYPE_DEFAULT, READ and POLL). An interesting optimization would be 
to combine the completion queues for at least the DEFAULT and READ queue 
types. Introducing a vop to configure the completion queue ID would make 
it almost impossible to implement this optimization in a generic way.

Asking to add a vop that improves performance by only a few percent for 
a *nonstandard* controller and at the same time that makes it very hard 
to optimize the driver for standards compliant controllers is something 
that I consider unreasonable.

Bart.