[PATCH v4 0/9] UFS patches for kernel 6.11

[PATCH v4 0/9] UFS patches for kernel 6.11

[Bart Van Assche]

Hi Martin,

Please consider this series of UFS driver patches for the next merge window.

Thanks,

Bart.

Changes compared to v3:
 - In patch 9/9, enable MCQ before reading the maximum number of active commands
   (MAC).
 - Added two refactoring patches: "Inline is_mcq_enabled()" and "Move the
   ufshcd_mcq_enable() call".

Changes compared to v2:
 - In patch 1/7, remove more duplicate declarations.

Changes compared to v1:
 - Based upon reviewer feedback, a new patch that renames the
   MASK_TRANSFER_REQUESTS_SLOTS constant has been added.
 - Because there is no agreement about these patches, the following three
   patches have been left out: "Make ufshcd_poll() complain about unsupported
   arguments", "Make the polling code report which command has been completed"
   and also "Check for completion from the timeout handler".

Bart Van Assche (9):
  scsi: ufs: Declare functions once
  scsi: ufs: Initialize struct uic_command once
  scsi: ufs: Remove two constants
  scsi: ufs: Rename the MASK_TRANSFER_REQUESTS_SLOTS constant
  scsi: ufs: Initialize hba->reserved_slot earlier
  scsi: ufs: Inline is_mcq_enabled()
  scsi: ufs: Move the ufshcd_mcq_enable() call
  scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()
  scsi: ufs: Make .get_hba_mac() optional

 drivers/ufs/core/ufs-mcq.c     |  24 +++++--
 drivers/ufs/core/ufshcd-priv.h |  15 +----
 drivers/ufs/core/ufshcd.c      | 112 +++++++++++++++++----------------
 include/ufs/ufshcd.h           |  13 ++--
 include/ufs/ufshci.h           |   3 +-
 5 files changed, 83 insertions(+), 84 deletions(-)

[PATCH v4 1/9] scsi: ufs: Declare functions once

[Bart Van Assche]

Several functions are declared in include/ufs/ufshcd.h and also in
drivers/ufs/core/ufshcd-priv.h. Remove the duplicate declarations.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd-priv.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..668748477e6e 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -66,14 +66,8 @@ 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);
-u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
-void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
 struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 					   struct request *req);
-unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
-				       struct ufs_hw_queue *hwq);
 void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 				    struct ufs_hw_queue *hwq);
 bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd);

[PATCH v4 2/9] scsi: ufs: Initialize struct uic_command once

[Bart Van Assche]

Instead of first zero-initializing struct uic_command and next initializing
it memberwise, initialize all members at once.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 61 ++++++++++++++++++++-------------------
 include/ufs/ufshcd.h      |  4 +--
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1fc08e34aaf9..b7ceedba4f93 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3993,11 +3993,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
  */
 static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_LINK_STARTUP,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_LINK_STARTUP;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_dbg(hba->dev,
@@ -4015,11 +4015,11 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  */
 static int ufshcd_dme_reset(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_RESET,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_RESET;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_err(hba->dev,
@@ -4054,11 +4054,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_configure_adapt);
  */
 static int ufshcd_dme_enable(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_ENABLE,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_ENABLE;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_err(hba->dev,
@@ -4111,7 +4111,12 @@ static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
 int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 			u8 attr_set, u32 mib_val, u8 peer)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = peer ? UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET,
+		.argument1 = attr_sel,
+		.argument2 = UIC_ARG_ATTR_TYPE(attr_set),
+		.argument3 = mib_val,
+	};
 	static const char *const action[] = {
 		"dme-set",
 		"dme-peer-set"
@@ -4120,12 +4125,6 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 	int ret;
 	int retries = UFS_UIC_COMMAND_RETRIES;
 
-	uic_cmd.command = peer ?
-		UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
-	uic_cmd.argument1 = attr_sel;
-	uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
-	uic_cmd.argument3 = mib_val;
-
 	do {
 		/* for peer attributes we retry upon failure */
 		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
@@ -4155,7 +4154,10 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
 int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 			u32 *mib_val, u8 peer)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = peer ? UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET,
+		.argument1 = attr_sel,
+	};
 	static const char *const action[] = {
 		"dme-get",
 		"dme-peer-get"
@@ -4189,10 +4191,6 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 		}
 	}
 
-	uic_cmd.command = peer ?
-		UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
-	uic_cmd.argument1 = attr_sel;
-
 	do {
 		/* for peer attributes we retry upon failure */
 		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
@@ -4325,7 +4323,11 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
  */
 int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_SET,
+		.argument1 = UIC_ARG_MIB(PA_PWRMODE),
+		.argument3 = mode,
+	};
 	int ret;
 
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP) {
@@ -4338,9 +4340,6 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 		}
 	}
 
-	uic_cmd.command = UIC_CMD_DME_SET;
-	uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
-	uic_cmd.argument3 = mode;
 	ufshcd_hold(hba);
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	ufshcd_release(hba);
@@ -4381,13 +4380,14 @@ EXPORT_SYMBOL_GPL(ufshcd_link_recovery);
 
 int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 {
-	int ret;
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_HIBER_ENTER,
+	};
 	ktime_t start = ktime_get();
+	int ret;
 
 	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE);
 
-	uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "enter",
 			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
@@ -4405,13 +4405,14 @@ EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_enter);
 
 int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_HIBER_EXIT,
+	};
 	int ret;
 	ktime_t start = ktime_get();
 
 	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE);
 
-	uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "exit",
 			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9e0581115b34..d4d63507d090 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -73,8 +73,8 @@ enum ufs_event_type {
  * @done: UIC command completion
  */
 struct uic_command {
-	u32 command;
-	u32 argument1;
+	const u32 command;
+	const u32 argument1;
 	u32 argument2;
 	u32 argument3;
 	int cmd_active;

[PATCH v4 3/9] scsi: ufs: Remove two constants

[Bart Van Assche]

The SCSI host template members .cmd_per_lun and .can_queue are copied
into the SCSI host data structure. Before these are used, these are
overwritten by ufshcd_init(). Hence, this patch does not change any
functionality.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Keoseong Park <keosung.park@samsung.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b7ceedba4f93..9a0697556953 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -164,8 +164,6 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
 enum {
 	UFSHCD_MAX_CHANNEL	= 0,
 	UFSHCD_MAX_ID		= 1,
-	UFSHCD_CMD_PER_LUN	= 32 - UFSHCD_NUM_RESERVED,
-	UFSHCD_CAN_QUEUE	= 32 - UFSHCD_NUM_RESERVED,
 };
 
 static const char *const ufshcd_state_name[] = {
@@ -8958,8 +8956,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.eh_timed_out		= ufshcd_eh_timed_out,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
-	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-	.can_queue		= UFSHCD_CAN_QUEUE,
 	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
 	.max_sectors		= SZ_1M / SECTOR_SIZE,
 	.max_host_blocked	= 1,

[PATCH v4 4/9] scsi: ufs: Rename the MASK_TRANSFER_REQUESTS_SLOTS constant

[Bart Van Assche]

Rename this constant to prepare for the introduction of the
MASK_TRANSFER_REQUESTS_SLOTS_MCQ constant. The acronym "SDB" stands for
"single doorbell" (mode).

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 include/ufs/ufshci.h      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9a0697556953..2cbd0f91953b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2401,7 +2401,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
 		hba->capabilities &= ~MASK_64_ADDRESSING_SUPPORT;
 
 	/* nutrs and nutmrs are 0 based values */
-	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
+	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_SDB) + 1;
 	hba->nutmrs =
 	((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
 	hba->reserved_slot = hba->nutrs - 1;
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index c50f92bf2e1d..8d0cc73537c6 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -67,7 +67,7 @@ enum {
 
 /* Controller capability masks */
 enum {
-	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
+	MASK_TRANSFER_REQUESTS_SLOTS_SDB	= 0x0000001F,
 	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
 	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
 	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

[PATCH v4 5/9] scsi: ufs: Initialize hba->reserved_slot earlier

[Bart Van Assche]

Move the hba->reserved_slot and the host->can_queue assignments from
ufshcd_config_mcq() into ufshcd_alloc_mcq(). The advantages of this
change are as follows:
- It becomes easier to verify that these two parameters are updated
  if hba->nutrs is updated.
- It prevents unnecessary assignments to these two parameters. While
  ufshcd_config_mcq() is called during host reset, ufshcd_alloc_mcq()
  is not.

Cc: Can Guo <quic_cang@quicinc.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2cbd0f91953b..178b0abaeb30 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8678,6 +8678,9 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba)
 	if (ret)
 		goto err;
 
+	hba->host->can_queue = hba->nutrs - UFSHCD_NUM_RESERVED;
+	hba->reserved_slot = hba->nutrs - UFSHCD_NUM_RESERVED;
+
 	return 0;
 err:
 	hba->nutrs = old_nutrs;
@@ -8699,9 +8702,6 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
 	ufshcd_mcq_make_queues_operational(hba);
 	ufshcd_mcq_config_mac(hba, hba->nutrs);
 
-	hba->host->can_queue = hba->nutrs - UFSHCD_NUM_RESERVED;
-	hba->reserved_slot = hba->nutrs - UFSHCD_NUM_RESERVED;
-
 	ufshcd_mcq_enable(hba);
 	hba->mcq_enabled = true;
 

[PATCH v4 6/9] scsi: ufs: Inline is_mcq_enabled()

[Bart Van Assche]

Improve code readability by inlining is_mcq_enabled().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 28 ++++++++++++++--------------
 include/ufs/ufshcd.h      |  5 -----
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 178b0abaeb30..4c138f42a802 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -453,7 +453,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 
 	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
 
 		hwq_id = hwq->id;
@@ -2301,7 +2301,7 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
 	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 		ufshcd_start_monitor(hba, lrbp);
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		int utrd_size = sizeof(struct utp_transfer_req_desc);
 		struct utp_transfer_req_desc *src = lrbp->utr_descriptor_ptr;
 		struct utp_transfer_req_desc *dest;
@@ -3000,7 +3000,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		goto out;
 	}
 
-	if (is_mcq_enabled(hba))
+	if (hba->mcq_enabled)
 		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd));
 
 	ufshcd_send_command(hba, tag, hwq);
@@ -3059,7 +3059,7 @@ static int ufshcd_clear_cmd(struct ufs_hba *hba, u32 task_tag)
 	unsigned long flags;
 	int err;
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		/*
 		 * MCQ mode. Clean up the MCQ resources similar to
 		 * what the ufshcd_utrl_clear() does for SDB mode.
@@ -3169,7 +3169,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 			__func__, lrbp->task_tag);
 
 		/* MCQ mode */
-		if (is_mcq_enabled(hba)) {
+		if (hba->mcq_enabled) {
 			/* successfully cleared the command, retry if needed */
 			if (ufshcd_clear_cmd(hba, lrbp->task_tag) == 0)
 				err = -EAGAIN;
@@ -5560,7 +5560,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	u32 tr_doorbell;
 	struct ufs_hw_queue *hwq;
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		hwq = &hba->uhq[queue_num];
 
 		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
@@ -6201,7 +6201,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 /* Complete requests that have door-bell cleared */
 static void ufshcd_complete_requests(struct ufs_hba *hba, bool force_compl)
 {
-	if (is_mcq_enabled(hba))
+	if (hba->mcq_enabled)
 		ufshcd_mcq_compl_pending_transfer(hba, force_compl);
 	else
 		ufshcd_transfer_req_compl(hba);
@@ -6458,7 +6458,7 @@ static bool ufshcd_abort_one(struct request *rq, void *priv)
 		*ret ? "failed" : "succeeded");
 
 	/* Release cmd in MCQ mode if abort succeeds */
-	if (is_mcq_enabled(hba) && (*ret == 0)) {
+	if (hba->mcq_enabled && (*ret == 0)) {
 		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(lrbp->cmd));
 		spin_lock_irqsave(&hwq->cq_lock, flags);
 		if (ufshcd_cmd_inflight(lrbp->cmd))
@@ -7389,7 +7389,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 		goto out;
 	}
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		for (pos = 0; pos < hba->nutrs; pos++) {
 			lrbp = &hba->lrb[pos];
 			if (ufshcd_cmd_inflight(lrbp->cmd) &&
@@ -7485,7 +7485,7 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
 			 */
 			dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n",
 				__func__, tag);
-			if (is_mcq_enabled(hba)) {
+			if (hba->mcq_enabled) {
 				/* MCQ mode */
 				if (ufshcd_cmd_inflight(lrbp->cmd)) {
 					/* sleep for max. 200us same delay as in SDB mode */
@@ -7563,7 +7563,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 
 	ufshcd_hold(hba);
 
-	if (!is_mcq_enabled(hba)) {
+	if (!hba->mcq_enabled) {
 		reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 		if (!test_bit(tag, &hba->outstanding_reqs)) {
 			/* If command is already aborted/completed, return FAILED. */
@@ -7596,7 +7596,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	}
 	hba->req_abort_count++;
 
-	if (!is_mcq_enabled(hba) && !(reg & (1 << tag))) {
+	if (!hba->mcq_enabled && !(reg & (1 << tag))) {
 		/* only execute this code in single doorbell mode */
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
@@ -7623,7 +7623,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		goto release;
 	}
 
-	if (is_mcq_enabled(hba)) {
+	if (hba->mcq_enabled) {
 		/* MCQ mode. Branch off to handle abort for mcq mode */
 		err = ufshcd_mcq_abort(cmd);
 		goto release;
@@ -8732,7 +8732,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	ufshcd_set_link_active(hba);
 
 	/* Reconfigure MCQ upon reset */
-	if (is_mcq_enabled(hba) && !init_dev_params)
+	if (hba->mcq_enabled && !init_dev_params)
 		ufshcd_config_mcq(hba);
 
 	/* Verify device initialization by sending NOP OUT UPIU */
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d4d63507d090..c0e28a512b3c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1132,11 +1132,6 @@ struct ufs_hw_queue {
 
 #define MCQ_QCFG_SIZE		0x40
 
-static inline bool is_mcq_enabled(struct ufs_hba *hba)
-{
-	return hba->mcq_enabled;
-}
-
 static inline unsigned int ufshcd_mcq_opr_offset(struct ufs_hba *hba,
 		enum ufshcd_mcq_opr opr, int idx)
 {

[PATCH v4 7/9] scsi: ufs: Move the ufshcd_mcq_enable() call

[Bart Van Assche]

Move the ufshcd_mcq_enable() call and also the hba->mcq_enabled = true
assignment from inside ufshcd_config_mcq() to the callers of this function.
No functionality is changed by this patch.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4c138f42a802..b3444f9ce130 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8702,9 +8702,6 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
 	ufshcd_mcq_make_queues_operational(hba);
 	ufshcd_mcq_config_mac(hba, hba->nutrs);
 
-	ufshcd_mcq_enable(hba);
-	hba->mcq_enabled = true;
-
 	dev_info(hba->dev, "MCQ configured, nr_queues=%d, io_queues=%d, read_queue=%d, poll_queues=%d, queue_depth=%d\n",
 		 hba->nr_hw_queues, hba->nr_queues[HCTX_TYPE_DEFAULT],
 		 hba->nr_queues[HCTX_TYPE_READ], hba->nr_queues[HCTX_TYPE_POLL],
@@ -8732,8 +8729,10 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	ufshcd_set_link_active(hba);
 
 	/* Reconfigure MCQ upon reset */
-	if (hba->mcq_enabled && !init_dev_params)
+	if (hba->mcq_enabled && !init_dev_params) {
 		ufshcd_config_mcq(hba);
+		ufshcd_mcq_enable(hba);
+	}
 
 	/* Verify device initialization by sending NOP OUT UPIU */
 	ret = ufshcd_verify_dev_init(hba);
@@ -8757,6 +8756,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 			ret = ufshcd_alloc_mcq(hba);
 			if (!ret) {
 				ufshcd_config_mcq(hba);
+				ufshcd_mcq_enable(hba);
+				hba->mcq_enabled = true;
 			} else {
 				/* Continue with SDB mode */
 				use_mcq_mode = false;
@@ -8772,6 +8773,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 		} else if (is_mcq_supported(hba)) {
 			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is set */
 			ufshcd_config_mcq(hba);
+			ufshcd_mcq_enable(hba);
+			hba->mcq_enabled = true;
 		}
 	}
 

[PATCH v4 8/9] scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()

[Bart Van Assche]

Make ufshcd_mcq_decide_queue_depth() easier to read by inlining
ufshcd_mcq_vops_get_hba_mac().

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c     | 18 +++++++++++-------
 drivers/ufs/core/ufshcd-priv.h |  8 --------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 4bcae410c268..0482c7a1e419 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -144,14 +144,14 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
  */
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 {
-	int mac;
+	int mac = -EOPNOTSUPP;
 
-	/* Mandatory to implement get_hba_mac() */
-	mac = ufshcd_mcq_vops_get_hba_mac(hba);
-	if (mac < 0) {
-		dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
-		return mac;
-	}
+	if (!hba->vops || !hba->vops->get_hba_mac)
+		goto err;
+
+	mac = hba->vops->get_hba_mac(hba);
+	if (mac < 0)
+		goto err;
 
 	WARN_ON_ONCE(!hba->dev_info.bqueuedepth);
 	/*
@@ -160,6 +160,10 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 	 * shared queuing architecture is enabled.
 	 */
 	return min_t(int, mac, hba->dev_info.bqueuedepth);
+
+err:
+	dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
+	return mac;
 }
 
 static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 668748477e6e..88ce93748305 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -249,14 +249,6 @@ static inline int ufshcd_vops_mcq_config_resource(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
-static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba)
-{
-	if (hba->vops && hba->vops->get_hba_mac)
-		return hba->vops->get_hba_mac(hba);
-
-	return -EOPNOTSUPP;
-}
-
 static inline int ufshcd_mcq_vops_op_runtime_config(struct ufs_hba *hba)
 {
 	if (hba->vops && hba->vops->op_runtime_config)

[PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
the maximum number of supported commands in the controller capabilities
register. Use that value if .get_hba_mac == NULL.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c     | 20 ++++++++++++++------
 drivers/ufs/core/ufshcd-priv.h |  1 +
 drivers/ufs/core/ufshcd.c      |  6 ++++--
 include/ufs/ufshcd.h           |  4 +++-
 include/ufs/ufshci.h           |  1 +
 5 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 0482c7a1e419..b2cf34a1fe48 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -138,18 +138,21 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
  *
  * MAC - Max. Active Command of the Host Controller (HC)
  * HC wouldn't send more than this commands to the device.
- * It is mandatory to implement get_hba_mac() to enable MCQ mode.
  * Calculates and adjusts the queue depth based on the depth
  * supported by the HC and ufs device.
  */
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 {
-	int mac = -EOPNOTSUPP;
+	int mac;
 
-	if (!hba->vops || !hba->vops->get_hba_mac)
-		goto err;
-
-	mac = hba->vops->get_hba_mac(hba);
+	if (!hba->vops || !hba->vops->get_hba_mac) {
+		hba->capabilities =
+			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
+		mac = hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_MCQ;
+		mac++;
+	} else {
+		mac = hba->vops->get_hba_mac(hba);
+	}
 	if (mac < 0)
 		goto err;
 
@@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL_GPL(ufshcd_mcq_enable);
 
+void ufshcd_mcq_disable(struct ufs_hba *hba)
+{
+	ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG);
+}
+
 void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg)
 {
 	ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 88ce93748305..ce36154ce963 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
 void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 			  struct cq_entry *cqe);
 int ufshcd_mcq_init(struct ufs_hba *hba);
+void ufshcd_mcq_disable(struct ufs_hba *hba);
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
 int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
 struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b3444f9ce130..9e0290c6c2d3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 		if (ret)
 			return ret;
 		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
+			ufshcd_mcq_enable(hba);
+			hba->mcq_enabled = true;
 			ret = ufshcd_alloc_mcq(hba);
 			if (!ret) {
 				ufshcd_config_mcq(hba);
-				ufshcd_mcq_enable(hba);
-				hba->mcq_enabled = true;
 			} else {
 				/* Continue with SDB mode */
+				ufshcd_mcq_disable(hba);
+				hba->mcq_enabled = false;
 				use_mcq_mode = false;
 				dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
 					 ret);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index c0e28a512b3c..6518997930f3 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
  * @event_notify: called to notify important events
  * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
  * @mcq_config_resource: called to configure MCQ platform resources
- * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
+ * @get_hba_mac: reports maximum number of outstanding commands supported by
+ *	the controller. Should be implemented for UFSHCI 4.0 or later
+ *	controllers that are not compliant with the UFSHCI 4.0 specification.
  * @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
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 8d0cc73537c6..38fe97971a65 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -68,6 +68,7 @@ enum {
 /* Controller capability masks */
 enum {
 	MASK_TRANSFER_REQUESTS_SLOTS_SDB	= 0x0000001F,
+	MASK_TRANSFER_REQUESTS_SLOTS_MCQ	= 0x000000FF,
 	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
 	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
 	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

Re: [PATCH v4 6/9] scsi: ufs: Inline is_mcq_enabled()

[Peter Wang (王信友)]

On Tue, 2024-07-02 at 13:39 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Improve code readability by inlining is_mcq_enabled().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 28 ++++++++++++++--------------
>  include/ufs/ufshcd.h      |  5 -----
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 178b0abaeb30..4c138f42a802 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -453,7 +453,7 @@ static void ufshcd_add_command_trace(struct
> ufs_hba *hba, unsigned int tag,
>  
>  	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba,
> rq);
>  
>  		hwq_id = hwq->id;
> @@ -2301,7 +2301,7 @@ void ufshcd_send_command(struct ufs_hba *hba,
> unsigned int task_tag,
>  	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>  		ufshcd_start_monitor(hba, lrbp);
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		int utrd_size = sizeof(struct utp_transfer_req_desc);
>  		struct utp_transfer_req_desc *src = lrbp-
> >utr_descriptor_ptr;
>  		struct utp_transfer_req_desc *dest;
> @@ -3000,7 +3000,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>  		goto out;
>  	}
>  
> -	if (is_mcq_enabled(hba))
> +	if (hba->mcq_enabled)
>  		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd));
>  
>  	ufshcd_send_command(hba, tag, hwq);
> @@ -3059,7 +3059,7 @@ static int ufshcd_clear_cmd(struct ufs_hba
> *hba, u32 task_tag)
>  	unsigned long flags;
>  	int err;
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		/*
>  		 * MCQ mode. Clean up the MCQ resources similar to
>  		 * what the ufshcd_utrl_clear() does for SDB mode.
> @@ -3169,7 +3169,7 @@ static int ufshcd_wait_for_dev_cmd(struct
> ufs_hba *hba,
>  			__func__, lrbp->task_tag);
>  
>  		/* MCQ mode */
> -		if (is_mcq_enabled(hba)) {
> +		if (hba->mcq_enabled) {
>  			/* successfully cleared the command, retry if
> needed */
>  			if (ufshcd_clear_cmd(hba, lrbp->task_tag) == 0)
>  				err = -EAGAIN;
> @@ -5560,7 +5560,7 @@ static int ufshcd_poll(struct Scsi_Host *shost,
> unsigned int queue_num)
>  	u32 tr_doorbell;
>  	struct ufs_hw_queue *hwq;
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		hwq = &hba->uhq[queue_num];
>  
>  		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
> @@ -6201,7 +6201,7 @@ static void
> ufshcd_exception_event_handler(struct work_struct *work)
>  /* Complete requests that have door-bell cleared */
>  static void ufshcd_complete_requests(struct ufs_hba *hba, bool
> force_compl)
>  {
> -	if (is_mcq_enabled(hba))
> +	if (hba->mcq_enabled)
>  		ufshcd_mcq_compl_pending_transfer(hba, force_compl);
>  	else
>  		ufshcd_transfer_req_compl(hba);
> @@ -6458,7 +6458,7 @@ static bool ufshcd_abort_one(struct request
> *rq, void *priv)
>  		*ret ? "failed" : "succeeded");
>  
>  	/* Release cmd in MCQ mode if abort succeeds */
> -	if (is_mcq_enabled(hba) && (*ret == 0)) {
> +	if (hba->mcq_enabled && (*ret == 0)) {
>  		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(lrbp-
> >cmd));
>  		spin_lock_irqsave(&hwq->cq_lock, flags);
>  		if (ufshcd_cmd_inflight(lrbp->cmd))
> @@ -7389,7 +7389,7 @@ static int
> ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
>  		goto out;
>  	}
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		for (pos = 0; pos < hba->nutrs; pos++) {
>  			lrbp = &hba->lrb[pos];
>  			if (ufshcd_cmd_inflight(lrbp->cmd) &&
> @@ -7485,7 +7485,7 @@ int ufshcd_try_to_abort_task(struct ufs_hba
> *hba, int tag)
>  			 */
>  			dev_err(hba->dev, "%s: cmd at tag %d not
> pending in the device.\n",
>  				__func__, tag);
> -			if (is_mcq_enabled(hba)) {
> +			if (hba->mcq_enabled) {
>  				/* MCQ mode */
>  				if (ufshcd_cmd_inflight(lrbp->cmd)) {
>  					/* sleep for max. 200us same
> delay as in SDB mode */
> @@ -7563,7 +7563,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  
>  	ufshcd_hold(hba);
>  
> -	if (!is_mcq_enabled(hba)) {
> +	if (!hba->mcq_enabled) {
>  		reg = ufshcd_readl(hba,
> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  		if (!test_bit(tag, &hba->outstanding_reqs)) {
>  			/* If command is already aborted/completed,
> return FAILED. */
> @@ -7596,7 +7596,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	}
>  	hba->req_abort_count++;
>  
> -	if (!is_mcq_enabled(hba) && !(reg & (1 << tag))) {
> +	if (!hba->mcq_enabled && !(reg & (1 << tag))) {
>  		/* only execute this code in single doorbell mode */
>  		dev_err(hba->dev,
>  		"%s: cmd was completed, but without a notifying intr,
> tag = %d",
> @@ -7623,7 +7623,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		goto release;
>  	}
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		/* MCQ mode. Branch off to handle abort for mcq mode */
>  		err = ufshcd_mcq_abort(cmd);
>  		goto release;
> @@ -8732,7 +8732,7 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  	ufshcd_set_link_active(hba);
>  
>  	/* Reconfigure MCQ upon reset */
> -	if (is_mcq_enabled(hba) && !init_dev_params)
> +	if (hba->mcq_enabled && !init_dev_params)
>  		ufshcd_config_mcq(hba);
>  
>  	/* Verify device initialization by sending NOP OUT UPIU */
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d4d63507d090..c0e28a512b3c 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1132,11 +1132,6 @@ struct ufs_hw_queue {
>  
>  #define MCQ_QCFG_SIZE		0x40
>  
> -static inline bool is_mcq_enabled(struct ufs_hba *hba)
> -{
> -	return hba->mcq_enabled;
> -}
> -
>  static inline unsigned int ufshcd_mcq_opr_offset(struct ufs_hba
> *hba,
>  		enum ufshcd_mcq_opr opr, int idx)
>  {

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v4 7/9] scsi: ufs: Move the ufshcd_mcq_enable() call

[Peter Wang (王信友)]

On Tue, 2024-07-02 at 13:39 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Move the ufshcd_mcq_enable() call and also the hba->mcq_enabled =
> true
> assignment from inside ufshcd_config_mcq() to the callers of this
> function.
> No functionality is changed by this patch.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 4c138f42a802..b3444f9ce130 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8702,9 +8702,6 @@ static void ufshcd_config_mcq(struct ufs_hba
> *hba)
>  	ufshcd_mcq_make_queues_operational(hba);
>  	ufshcd_mcq_config_mac(hba, hba->nutrs);
>  
> -	ufshcd_mcq_enable(hba);
> -	hba->mcq_enabled = true;
> -
>  	dev_info(hba->dev, "MCQ configured, nr_queues=%d, io_queues=%d,
> read_queue=%d, poll_queues=%d, queue_depth=%d\n",
>  		 hba->nr_hw_queues, hba->nr_queues[HCTX_TYPE_DEFAULT],
>  		 hba->nr_queues[HCTX_TYPE_READ], hba-
> >nr_queues[HCTX_TYPE_POLL],
> @@ -8732,8 +8729,10 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  	ufshcd_set_link_active(hba);
>  
>  	/* Reconfigure MCQ upon reset */
> -	if (hba->mcq_enabled && !init_dev_params)
> +	if (hba->mcq_enabled && !init_dev_params) {
>  		ufshcd_config_mcq(hba);
> +		ufshcd_mcq_enable(hba);
> +	}
>  
>  	/* Verify device initialization by sending NOP OUT UPIU */
>  	ret = ufshcd_verify_dev_init(hba);
> @@ -8757,6 +8756,8 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  			ret = ufshcd_alloc_mcq(hba);
>  			if (!ret) {
>  				ufshcd_config_mcq(hba);
> +				ufshcd_mcq_enable(hba);
> +				hba->mcq_enabled = true;
>  			} else {
>  				/* Continue with SDB mode */
>  				use_mcq_mode = false;
> @@ -8772,6 +8773,8 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  		} else if (is_mcq_supported(hba)) {
>  			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is
> set */
>  			ufshcd_config_mcq(hba);
> +			ufshcd_mcq_enable(hba);
> +			hba->mcq_enabled = true;
>  		}
>  	}
>  

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Peter Wang (王信友)]

On Tue, 2024-07-02 at 13:39 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  UFSHCI controllers that are compliant with the UFSHCI 4.0 standard
> report
> the maximum number of supported commands in the controller
> capabilities
> register. Use that value if .get_hba_mac == NULL.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufs-mcq.c     | 20 ++++++++++++++------
>  drivers/ufs/core/ufshcd-priv.h |  1 +
>  drivers/ufs/core/ufshcd.c      |  6 ++++--
>  include/ufs/ufshcd.h           |  4 +++-
>  include/ufs/ufshci.h           |  1 +
>  5 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 0482c7a1e419..b2cf34a1fe48 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -138,18 +138,21 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
>   *
>   * MAC - Max. Active Command of the Host Controller (HC)
>   * HC wouldn't send more than this commands to the device.
> - * It is mandatory to implement get_hba_mac() to enable MCQ mode.
>   * Calculates and adjusts the queue depth based on the depth
>   * supported by the HC and ufs device.
>   */
>  int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>  {
> -	int mac = -EOPNOTSUPP;
> +	int mac;
>  
> -	if (!hba->vops || !hba->vops->get_hba_mac)
> -		goto err;
> -
> -	mac = hba->vops->get_hba_mac(hba);
> +	if (!hba->vops || !hba->vops->get_hba_mac) {
> +		hba->capabilities =
> +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> +		mac = hba->capabilities &
> MASK_TRANSFER_REQUESTS_SLOTS_MCQ;
> +		mac++;
> +	} else {
> +		mac = hba->vops->get_hba_mac(hba);
> +	}
>  	if (mac < 0)
>  		goto err;
>  
> @@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba)
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_mcq_enable);
>  
> +void ufshcd_mcq_disable(struct ufs_hba *hba)
> +{
> +	ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG);
> +}
> +
>  void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg)
>  {
>  	ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA);
> diff --git a/drivers/ufs/core/ufshcd-priv.h
> b/drivers/ufs/core/ufshcd-priv.h
> index 88ce93748305..ce36154ce963 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba
> *hba, u32 ahit);
>  void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>  			  struct cq_entry *cqe);
>  int ufshcd_mcq_init(struct ufs_hba *hba);
> +void ufshcd_mcq_disable(struct ufs_hba *hba);
>  int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
>  int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
>  struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b3444f9ce130..9e0290c6c2d3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  		if (ret)
>  			return ret;
>  		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
> +			ufshcd_mcq_enable(hba);
> +			hba->mcq_enabled = true;
>  			ret = ufshcd_alloc_mcq(hba);
>  			if (!ret) {
>  				ufshcd_config_mcq(hba);
> -				ufshcd_mcq_enable(hba);
> -				hba->mcq_enabled = true;
>  			} else {
>  				/* Continue with SDB mode */
> +				ufshcd_mcq_disable(hba);
> +				hba->mcq_enabled = false;
>  				use_mcq_mode = false;
>  				dev_err(hba->dev, "MCQ mode is
> disabled, err=%d\n",
>  					 ret);
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index c0e28a512b3c..6518997930f3 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
>   * @event_notify: called to notify important events
>   * @reinit_notify: called to notify reinit of UFSHCD during max gear
> switch
>   * @mcq_config_resource: called to configure MCQ platform resources
> - * @get_hba_mac: called to get vendor specific mac value, mandatory
> for mcq mode
> + * @get_hba_mac: reports maximum number of outstanding commands
> supported by
> + *	the controller. Should be implemented for UFSHCI 4.0 or later
> + *	controllers that are not compliant with the UFSHCI 4.0
> specification.
>   * @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
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index 8d0cc73537c6..38fe97971a65 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -68,6 +68,7 @@ enum {
>  /* Controller capability masks */
>  enum {
>  	MASK_TRANSFER_REQUESTS_SLOTS_SDB	= 0x0000001F,
> +	MASK_TRANSFER_REQUESTS_SLOTS_MCQ	= 0x000000FF,
>  	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
>  	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
>  	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v4 4/9] scsi: ufs: Rename the MASK_TRANSFER_REQUESTS_SLOTS constant

[Manivannan Sadhasivam]

On Tue, Jul 02, 2024 at 01:39:12PM -0700, Bart Van Assche wrote:
> Rename this constant to prepare for the introduction of the
> MASK_TRANSFER_REQUESTS_SLOTS_MCQ constant. The acronym "SDB" stands for
> "single doorbell" (mode).
> 
> Reviewed-by: Peter Wang <peter.wang@mediatek.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 2 +-
>  include/ufs/ufshci.h      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 9a0697556953..2cbd0f91953b 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2401,7 +2401,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
>  		hba->capabilities &= ~MASK_64_ADDRESSING_SUPPORT;
>  
>  	/* nutrs and nutmrs are 0 based values */
> -	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
> +	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_SDB) + 1;
>  	hba->nutmrs =
>  	((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
>  	hba->reserved_slot = hba->nutrs - 1;
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index c50f92bf2e1d..8d0cc73537c6 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -67,7 +67,7 @@ enum {
>  
>  /* Controller capability masks */
>  enum {
> -	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
> +	MASK_TRANSFER_REQUESTS_SLOTS_SDB	= 0x0000001F,
>  	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
>  	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
>  	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 5/9] scsi: ufs: Initialize hba->reserved_slot earlier

[Manivannan Sadhasivam]

On Tue, Jul 02, 2024 at 01:39:13PM -0700, Bart Van Assche wrote:
> Move the hba->reserved_slot and the host->can_queue assignments from
> ufshcd_config_mcq() into ufshcd_alloc_mcq(). The advantages of this
> change are as follows:
> - It becomes easier to verify that these two parameters are updated
>   if hba->nutrs is updated.
> - It prevents unnecessary assignments to these two parameters. While
>   ufshcd_config_mcq() is called during host reset, ufshcd_alloc_mcq()
>   is not.
> 
> Cc: Can Guo <quic_cang@quicinc.com>
> Reviewed-by: Peter Wang <peter.wang@mediatek.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 2cbd0f91953b..178b0abaeb30 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8678,6 +8678,9 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba)
>  	if (ret)
>  		goto err;
>  
> +	hba->host->can_queue = hba->nutrs - UFSHCD_NUM_RESERVED;
> +	hba->reserved_slot = hba->nutrs - UFSHCD_NUM_RESERVED;
> +
>  	return 0;
>  err:
>  	hba->nutrs = old_nutrs;
> @@ -8699,9 +8702,6 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
>  	ufshcd_mcq_make_queues_operational(hba);
>  	ufshcd_mcq_config_mac(hba, hba->nutrs);
>  
> -	hba->host->can_queue = hba->nutrs - UFSHCD_NUM_RESERVED;
> -	hba->reserved_slot = hba->nutrs - UFSHCD_NUM_RESERVED;
> -
>  	ufshcd_mcq_enable(hba);
>  	hba->mcq_enabled = true;
>  

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 6/9] scsi: ufs: Inline is_mcq_enabled()

[Manivannan Sadhasivam]

On Tue, Jul 02, 2024 at 01:39:14PM -0700, Bart Van Assche wrote:
> Improve code readability by inlining is_mcq_enabled().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 28 ++++++++++++++--------------
>  include/ufs/ufshcd.h      |  5 -----
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 178b0abaeb30..4c138f42a802 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -453,7 +453,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
>  
>  	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
>  
>  		hwq_id = hwq->id;
> @@ -2301,7 +2301,7 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
>  	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>  		ufshcd_start_monitor(hba, lrbp);
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		int utrd_size = sizeof(struct utp_transfer_req_desc);
>  		struct utp_transfer_req_desc *src = lrbp->utr_descriptor_ptr;
>  		struct utp_transfer_req_desc *dest;
> @@ -3000,7 +3000,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  		goto out;
>  	}
>  
> -	if (is_mcq_enabled(hba))
> +	if (hba->mcq_enabled)
>  		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd));
>  
>  	ufshcd_send_command(hba, tag, hwq);
> @@ -3059,7 +3059,7 @@ static int ufshcd_clear_cmd(struct ufs_hba *hba, u32 task_tag)
>  	unsigned long flags;
>  	int err;
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		/*
>  		 * MCQ mode. Clean up the MCQ resources similar to
>  		 * what the ufshcd_utrl_clear() does for SDB mode.
> @@ -3169,7 +3169,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
>  			__func__, lrbp->task_tag);
>  
>  		/* MCQ mode */
> -		if (is_mcq_enabled(hba)) {
> +		if (hba->mcq_enabled) {
>  			/* successfully cleared the command, retry if needed */
>  			if (ufshcd_clear_cmd(hba, lrbp->task_tag) == 0)
>  				err = -EAGAIN;
> @@ -5560,7 +5560,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
>  	u32 tr_doorbell;
>  	struct ufs_hw_queue *hwq;
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		hwq = &hba->uhq[queue_num];
>  
>  		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
> @@ -6201,7 +6201,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>  /* Complete requests that have door-bell cleared */
>  static void ufshcd_complete_requests(struct ufs_hba *hba, bool force_compl)
>  {
> -	if (is_mcq_enabled(hba))
> +	if (hba->mcq_enabled)
>  		ufshcd_mcq_compl_pending_transfer(hba, force_compl);
>  	else
>  		ufshcd_transfer_req_compl(hba);
> @@ -6458,7 +6458,7 @@ static bool ufshcd_abort_one(struct request *rq, void *priv)
>  		*ret ? "failed" : "succeeded");
>  
>  	/* Release cmd in MCQ mode if abort succeeds */
> -	if (is_mcq_enabled(hba) && (*ret == 0)) {
> +	if (hba->mcq_enabled && (*ret == 0)) {
>  		hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(lrbp->cmd));
>  		spin_lock_irqsave(&hwq->cq_lock, flags);
>  		if (ufshcd_cmd_inflight(lrbp->cmd))
> @@ -7389,7 +7389,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
>  		goto out;
>  	}
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		for (pos = 0; pos < hba->nutrs; pos++) {
>  			lrbp = &hba->lrb[pos];
>  			if (ufshcd_cmd_inflight(lrbp->cmd) &&
> @@ -7485,7 +7485,7 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
>  			 */
>  			dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n",
>  				__func__, tag);
> -			if (is_mcq_enabled(hba)) {
> +			if (hba->mcq_enabled) {
>  				/* MCQ mode */
>  				if (ufshcd_cmd_inflight(lrbp->cmd)) {
>  					/* sleep for max. 200us same delay as in SDB mode */
> @@ -7563,7 +7563,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  
>  	ufshcd_hold(hba);
>  
> -	if (!is_mcq_enabled(hba)) {
> +	if (!hba->mcq_enabled) {
>  		reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  		if (!test_bit(tag, &hba->outstanding_reqs)) {
>  			/* If command is already aborted/completed, return FAILED. */
> @@ -7596,7 +7596,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	}
>  	hba->req_abort_count++;
>  
> -	if (!is_mcq_enabled(hba) && !(reg & (1 << tag))) {
> +	if (!hba->mcq_enabled && !(reg & (1 << tag))) {
>  		/* only execute this code in single doorbell mode */
>  		dev_err(hba->dev,
>  		"%s: cmd was completed, but without a notifying intr, tag = %d",
> @@ -7623,7 +7623,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		goto release;
>  	}
>  
> -	if (is_mcq_enabled(hba)) {
> +	if (hba->mcq_enabled) {
>  		/* MCQ mode. Branch off to handle abort for mcq mode */
>  		err = ufshcd_mcq_abort(cmd);
>  		goto release;
> @@ -8732,7 +8732,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  	ufshcd_set_link_active(hba);
>  
>  	/* Reconfigure MCQ upon reset */
> -	if (is_mcq_enabled(hba) && !init_dev_params)
> +	if (hba->mcq_enabled && !init_dev_params)
>  		ufshcd_config_mcq(hba);
>  
>  	/* Verify device initialization by sending NOP OUT UPIU */
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d4d63507d090..c0e28a512b3c 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1132,11 +1132,6 @@ struct ufs_hw_queue {
>  
>  #define MCQ_QCFG_SIZE		0x40
>  
> -static inline bool is_mcq_enabled(struct ufs_hba *hba)
> -{
> -	return hba->mcq_enabled;
> -}
> -
>  static inline unsigned int ufshcd_mcq_opr_offset(struct ufs_hba *hba,
>  		enum ufshcd_mcq_opr opr, int idx)
>  {

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 7/9] scsi: ufs: Move the ufshcd_mcq_enable() call

[Manivannan Sadhasivam]

On Tue, Jul 02, 2024 at 01:39:15PM -0700, Bart Van Assche wrote:
> Move the ufshcd_mcq_enable() call and also the hba->mcq_enabled = true
> assignment from inside ufshcd_config_mcq() to the callers of this function.
> No functionality is changed by this patch.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

For the code,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

But neither the subject, nor the commit message explains _why_ this change is
needed.

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 4c138f42a802..b3444f9ce130 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8702,9 +8702,6 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
>  	ufshcd_mcq_make_queues_operational(hba);
>  	ufshcd_mcq_config_mac(hba, hba->nutrs);
>  
> -	ufshcd_mcq_enable(hba);
> -	hba->mcq_enabled = true;
> -
>  	dev_info(hba->dev, "MCQ configured, nr_queues=%d, io_queues=%d, read_queue=%d, poll_queues=%d, queue_depth=%d\n",
>  		 hba->nr_hw_queues, hba->nr_queues[HCTX_TYPE_DEFAULT],
>  		 hba->nr_queues[HCTX_TYPE_READ], hba->nr_queues[HCTX_TYPE_POLL],
> @@ -8732,8 +8729,10 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  	ufshcd_set_link_active(hba);
>  
>  	/* Reconfigure MCQ upon reset */
> -	if (hba->mcq_enabled && !init_dev_params)
> +	if (hba->mcq_enabled && !init_dev_params) {
>  		ufshcd_config_mcq(hba);
> +		ufshcd_mcq_enable(hba);
> +	}
>  
>  	/* Verify device initialization by sending NOP OUT UPIU */
>  	ret = ufshcd_verify_dev_init(hba);
> @@ -8757,6 +8756,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  			ret = ufshcd_alloc_mcq(hba);
>  			if (!ret) {
>  				ufshcd_config_mcq(hba);
> +				ufshcd_mcq_enable(hba);
> +				hba->mcq_enabled = true;
>  			} else {
>  				/* Continue with SDB mode */
>  				use_mcq_mode = false;
> @@ -8772,6 +8773,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  		} else if (is_mcq_supported(hba)) {
>  			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is set */
>  			ufshcd_config_mcq(hba);
> +			ufshcd_mcq_enable(hba);
> +			hba->mcq_enabled = true;
>  		}
>  	}
>  

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

On Tue, Jul 02, 2024 at 01:39:17PM -0700, Bart Van Assche wrote:
> UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
> the maximum number of supported commands in the controller capabilities
> register. Use that value if .get_hba_mac == NULL.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufs-mcq.c     | 20 ++++++++++++++------
>  drivers/ufs/core/ufshcd-priv.h |  1 +
>  drivers/ufs/core/ufshcd.c      |  6 ++++--
>  include/ufs/ufshcd.h           |  4 +++-
>  include/ufs/ufshci.h           |  1 +
>  5 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 0482c7a1e419..b2cf34a1fe48 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -138,18 +138,21 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
>   *
>   * MAC - Max. Active Command of the Host Controller (HC)
>   * HC wouldn't send more than this commands to the device.
> - * It is mandatory to implement get_hba_mac() to enable MCQ mode.
>   * Calculates and adjusts the queue depth based on the depth
>   * supported by the HC and ufs device.
>   */
>  int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>  {
> -	int mac = -EOPNOTSUPP;
> +	int mac;
>  
> -	if (!hba->vops || !hba->vops->get_hba_mac)
> -		goto err;
> -
> -	mac = hba->vops->get_hba_mac(hba);
> +	if (!hba->vops || !hba->vops->get_hba_mac) {
> +		hba->capabilities =
> +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> +		mac = hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_MCQ;
> +		mac++;

Can you add a comment to state that the MAC value is 0 based?

> +	} else {
> +		mac = hba->vops->get_hba_mac(hba);
> +	}
>  	if (mac < 0)
>  		goto err;
>  
> @@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba)
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_mcq_enable);
>  
> +void ufshcd_mcq_disable(struct ufs_hba *hba)
> +{
> +	ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG);
> +}
> +
>  void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg)
>  {
>  	ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA);
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 88ce93748305..ce36154ce963 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
>  void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>  			  struct cq_entry *cqe);
>  int ufshcd_mcq_init(struct ufs_hba *hba);
> +void ufshcd_mcq_disable(struct ufs_hba *hba);
>  int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
>  int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
>  struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b3444f9ce130..9e0290c6c2d3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  		if (ret)
>  			return ret;
>  		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
> +			ufshcd_mcq_enable(hba);
> +			hba->mcq_enabled = true;

If the 'mcq_enabled' assignment goes hand in hand with
ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside?

- Mani

>  			ret = ufshcd_alloc_mcq(hba);
>  			if (!ret) {
>  				ufshcd_config_mcq(hba);
> -				ufshcd_mcq_enable(hba);
> -				hba->mcq_enabled = true;
>  			} else {
>  				/* Continue with SDB mode */
> +				ufshcd_mcq_disable(hba);
> +				hba->mcq_enabled = false;
>  				use_mcq_mode = false;
>  				dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
>  					 ret);
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index c0e28a512b3c..6518997930f3 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
>   * @event_notify: called to notify important events
>   * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
>   * @mcq_config_resource: called to configure MCQ platform resources
> - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
> + * @get_hba_mac: reports maximum number of outstanding commands supported by
> + *	the controller. Should be implemented for UFSHCI 4.0 or later
> + *	controllers that are not compliant with the UFSHCI 4.0 specification.
>   * @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
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index 8d0cc73537c6..38fe97971a65 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -68,6 +68,7 @@ enum {
>  /* Controller capability masks */
>  enum {
>  	MASK_TRANSFER_REQUESTS_SLOTS_SDB	= 0x0000001F,
> +	MASK_TRANSFER_REQUESTS_SLOTS_MCQ	= 0x000000FF,
>  	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
>  	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
>  	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 7/9] scsi: ufs: Move the ufshcd_mcq_enable() call

[Bart Van Assche]

On 7/3/24 6:10 AM, Manivannan Sadhasivam wrote:
> But neither the subject, nor the commit message explains _why_ this change is
> needed.

Right, I should have mentioned why I came up with this patch. If I have
to repost this patch series, I will make it clear in the patch
description that this patch is a refactoring patch that makes a later
patch easier to read ("scsi: ufs: Make .get_hba_mac() optional").

Thanks,

Bart.

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

On 7/3/24 6:22 AM, Manivannan Sadhasivam wrote:
> On Tue, Jul 02, 2024 at 01:39:17PM -0700, Bart Van Assche wrote:
>> -	mac = hba->vops->get_hba_mac(hba);
>> +	if (!hba->vops || !hba->vops->get_hba_mac) {
>> +		hba->capabilities =
>> +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
>> +		mac = hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_MCQ;
>> +		mac++;
> 
> Can you add a comment to state that the MAC value is 0 based?

Sure.

>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index b3444f9ce130..9e0290c6c2d3 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>>   		if (ret)
>>   			return ret;
>>   		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
>> +			ufshcd_mcq_enable(hba);
>> +			hba->mcq_enabled = true;
> 
> If the 'mcq_enabled' assignment goes hand in hand with
> ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside?

If an UFSHCI controller is reset, the controller is reset from MCQ mode
to SDB mode and it is derived from the hba->mcq_enabled structure member
that MCQ was enabled before the reset. In other words, moving all
hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
break the code that resets the UFSHCI controller.

Thanks,

Bart.

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
> On 7/3/24 6:22 AM, Manivannan Sadhasivam wrote:
> > On Tue, Jul 02, 2024 at 01:39:17PM -0700, Bart Van Assche wrote:
> > > -	mac = hba->vops->get_hba_mac(hba);
> > > +	if (!hba->vops || !hba->vops->get_hba_mac) {
> > > +		hba->capabilities =
> > > +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> > > +		mac = hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS_MCQ;
> > > +		mac++;
> > 
> > Can you add a comment to state that the MAC value is 0 based?
> 
> Sure.
> 
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index b3444f9ce130..9e0290c6c2d3 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
> > >   		if (ret)
> > >   			return ret;
> > >   		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
> > > +			ufshcd_mcq_enable(hba);
> > > +			hba->mcq_enabled = true;
> > 
> > If the 'mcq_enabled' assignment goes hand in hand with
> > ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside?
> 
> If an UFSHCI controller is reset, the controller is reset from MCQ mode
> to SDB mode and it is derived from the hba->mcq_enabled structure member
> that MCQ was enabled before the reset. In other words, moving all
> hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
> break the code that resets the UFSHCI controller.
> 

Hmm, could you please point me to the code that does this? I tried looking for
it but couldn't spot.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote:
> On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
>> If an UFSHCI controller is reset, the controller is reset from MCQ mode
>> to SDB mode and it is derived from the hba->mcq_enabled structure member
>> that MCQ was enabled before the reset. In other words, moving all
>> hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
>> break the code that resets the UFSHCI controller.
> 
> Hmm, could you please point me to the code that does this? I tried looking for
> it but couldn't spot.

* There is no "hba->mcq_enabled = false;" statement anywhere in the UFS
   driver core. This shows that the reset code does not reset
   hba->mcq_enabled.
* From the UFSHCI specification, offset 300h, global config register:
   in the "reset" column I see 0h for the queue type (QT) bit. In other
   words, if a UFS host controller is reset, if MCQ is enabled (QT=1),
   it must be disabled (QT=0) until the application processor enables it
   again.

Bart.

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote:
> On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
> > > If an UFSHCI controller is reset, the controller is reset from MCQ mode
> > > to SDB mode and it is derived from the hba->mcq_enabled structure member
> > > that MCQ was enabled before the reset. In other words, moving all
> > > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
> > > break the code that resets the UFSHCI controller.
> > 
> > Hmm, could you please point me to the code that does this? I tried looking for
> > it but couldn't spot.
> 
> * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS
>   driver core. This shows that the reset code does not reset
>   hba->mcq_enabled.

Right. So this flag is used to reconfigure the MCQ mode upon HCI reset.
Previously we never disabled MCQ mode as well. But that is being changed by this
patch.

> * From the UFSHCI specification, offset 300h, global config register:
>   in the "reset" column I see 0h for the queue type (QT) bit. In other
>   words, if a UFS host controller is reset, if MCQ is enabled (QT=1),
>   it must be disabled (QT=0) until the application processor enables it
>   again.
> 

So this means, once the HCI is reset, the QT field will become '0' by default.
So if MCQ mode is supposed to be enabled, then driver has to explicitly enable
it.

But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this
patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to
understand why you cannot move the assignment to the enable/disable API itself.

If you do not set the flag after calling the ufshcd_mcq_disable() API, your
argument makes sense. But that's not the case. Perhaps I'm missing anything
obvious?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

On 7/8/24 3:44 AM, Manivannan Sadhasivam wrote:
> On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote:
>> On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote:
>>> On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
>>>> If an UFSHCI controller is reset, the controller is reset from MCQ mode
>>>> to SDB mode and it is derived from the hba->mcq_enabled structure member
>>>> that MCQ was enabled before the reset. In other words, moving all
>>>> hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
>>>> break the code that resets the UFSHCI controller.
>>>
>>> Hmm, could you please point me to the code that does this? I tried looking for
>>> it but couldn't spot.
>>
>> * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS
>>    driver core. This shows that the reset code does not reset
>>    hba->mcq_enabled.
> 
> Right. So this flag is used to reconfigure the MCQ mode upon HCI reset.
> Previously we never disabled MCQ mode as well. But that is being changed by this
> patch.

Only in an error path.

>> * From the UFSHCI specification, offset 300h, global config register:
>>    in the "reset" column I see 0h for the queue type (QT) bit. In other
>>    words, if a UFS host controller is reset, if MCQ is enabled (QT=1),
>>    it must be disabled (QT=0) until the application processor enables it
>>    again.
>>
> 
> So this means, once the HCI is reset, the QT field will become '0' by default.
> So if MCQ mode is supposed to be enabled, then driver has to explicitly enable
> it.
> 
> But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this
> patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to
> understand why you cannot move the assignment to the enable/disable API itself.
> 
> If you do not set the flag after calling the ufshcd_mcq_disable() API, your
> argument makes sense. But that's not the case. Perhaps I'm missing anything
> obvious?

What do you want? That I move the "hba->mcq_enabled = false;" statement
into ufshcd_mcq_disable()? That would be wrong because (a) it would
introduce a confusing behavior difference between ufshcd_mcq_enable()
(does not modify hba->mcq_enabled) and ufshcd_mcq_disable() (would
modify hba->mcq_enabled if I implement your proposal) and (b) wouldn't
reduce the code size because ufshcd_mcq_disable() only has one caller.

Bart.

Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

On Mon, Jul 08, 2024 at 10:10:43AM -0700, Bart Van Assche wrote:
> On 7/8/24 3:44 AM, Manivannan Sadhasivam wrote:
> > On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote:
> > > On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote:
> > > > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
> > > > > If an UFSHCI controller is reset, the controller is reset from MCQ mode
> > > > > to SDB mode and it is derived from the hba->mcq_enabled structure member
> > > > > that MCQ was enabled before the reset. In other words, moving all
> > > > > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
> > > > > break the code that resets the UFSHCI controller.
> > > > 
> > > > Hmm, could you please point me to the code that does this? I tried looking for
> > > > it but couldn't spot.
> > > 
> > > * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS
> > >    driver core. This shows that the reset code does not reset
> > >    hba->mcq_enabled.
> > 
> > Right. So this flag is used to reconfigure the MCQ mode upon HCI reset.
> > Previously we never disabled MCQ mode as well. But that is being changed by this
> > patch.
> 
> Only in an error path.
> 
> > > * From the UFSHCI specification, offset 300h, global config register:
> > >    in the "reset" column I see 0h for the queue type (QT) bit. In other
> > >    words, if a UFS host controller is reset, if MCQ is enabled (QT=1),
> > >    it must be disabled (QT=0) until the application processor enables it
> > >    again.
> > > 
> > 
> > So this means, once the HCI is reset, the QT field will become '0' by default.
> > So if MCQ mode is supposed to be enabled, then driver has to explicitly enable
> > it.
> > 
> > But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this
> > patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to
> > understand why you cannot move the assignment to the enable/disable API itself.
> > 
> > If you do not set the flag after calling the ufshcd_mcq_disable() API, your
> > argument makes sense. But that's not the case. Perhaps I'm missing anything
> > obvious?
> 
> What do you want? That I move the "hba->mcq_enabled = false;" statement
> into ufshcd_mcq_disable()? That would be wrong because (a) it would
> introduce a confusing behavior difference between ufshcd_mcq_enable()
> (does not modify hba->mcq_enabled) and ufshcd_mcq_disable() (would
> modify hba->mcq_enabled if I implement your proposal) and (b) wouldn't
> reduce the code size because ufshcd_mcq_disable() only has one caller.
> 

No, I'm not asking to move the assignment inside just the ufshcd_mcq_disable()
API, but for both. You are saying that doing so will break UFSHCI reset, but I
fail to understand how.

After your series applied, 'hba->mcq_enabled' is set to true in 2 places of
ufshcd.c. And in those 2 places, ufshcd_mcq_enable() is accompanied. There is
only one place you haven't added the assignment which is during the start of
ufshcd_device_init()::

        /* Reconfigure MCQ upon reset */
        if (hba->mcq_enabled && !init_dev_params) {
                ufshcd_config_mcq(hba);
                ufshcd_mcq_enable(hba);
        }

And this makes sense because, if mcq_enabled was already set to true, then you
are not setting the flag again. But even if you have moved the 'hba->mcq_enabled
= true' assignment inside ufshcd_mcq_enable(), it wouldn't cause any issues.

Where does the assignment actually breaking the reset code? This part I don't
understand.

- Mani

-- 
மணிவண்ணன் சதாசிவம்