diff mbox series

scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

Message ID 20230328103000.10757-1-powen.kao@mediatek.com (mailing list archive)
State New, archived
Headers show
Series scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter | expand

Commit Message

Po-Wen Kao March 28, 2023, 10:29 a.m. UTC
A dedicated queue for dev commands is not mandatory, hence let
UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
to allow sharing first hw queue for dev commands.

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

Comments

Stanley Jhu March 29, 2023, 1:29 a.m. UTC | #1
Hi, Powen,

On Tue, Mar 28, 2023 at 6:38 PM Po-Wen Kao <powen.kao@mediatek.com> wrote:
>
> A dedicated queue for dev commands is not mandatory, hence let
> UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
> to allow sharing first hw queue for dev commands.
>
> Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
> ---
>  drivers/ufs/core/ufs-mcq.c     | 35 +++++++++++++++++++++++++++-------
>  drivers/ufs/core/ufshcd-priv.h |  2 +-
>  drivers/ufs/core/ufshcd.c      |  2 +-
>  3 files changed, 30 insertions(+), 9 deletions(-)

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Bart Van Assche March 29, 2023, 3:17 a.m. UTC | #2
On 3/28/23 03:29, Po-Wen Kao wrote:
> A dedicated queue for dev commands is not mandatory, hence let
> UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
> to allow sharing first hw queue for dev commands.

Which queue is selected for device management commands?

What is the impact of this change? If a device command is queued on a 
queue with multiple pending commands, does that mean that it can take 
long for the device command to reach the UFS device?

The answer to the above questions should be in the patch description. 
Please expand the patch description.

> +unsigned int dev_cmd_queues = 1;
> +module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644);
> +MODULE_PARM_DESC(dev_cmd_queues,
> +		 "Number of queues used for dev command. Default value is 1");

I prefer a solution that does not require any new kernel module 
parameters. That means either a dedicated device command queue for all 
host controllers or no dedicated device command queue for any host 
controller.

Thanks,

Bart.
Po-Wen Kao March 30, 2023, 2:52 a.m. UTC | #3
On Tue, 2023-03-28 at 20:17 -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 3/28/23 03:29, Po-Wen Kao wrote:
> > A dedicated queue for dev commands is not mandatory, hence let
> > UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
> > to allow sharing first hw queue for dev commands.
> 
> Which queue is selected for device management commands? 

Queue 0 is selected by default.

@@ -423,8 +441,11 @@ int ufshcd_mcq_init(struct ufs_hba *hba)
 
        /* The very first HW queue serves device commands */
        hba->dev_cmd_queue = &hba->uhq[0];

> 
> What is the impact of this change? If a device command is queued on a
> queue with multiple pending commands, does that mean that it can take
> long for the device command to reach the UFS device?
> 
> The answer to the above questions should be in the patch description.
> Please expand the patch description.

I will make commit message clear in next update.

> > +unsigned int dev_cmd_queues = 1;
> > +module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops,
> > &dev_cmd_queues, 0644);
> > +MODULE_PARM_DESC(dev_cmd_queues,
> > +              "Number of queues used for dev command. Default
> > value is 1");
> 
> I prefer a solution that does not require any new kernel module
> parameters. That means either a dedicated device command queue for
> all
> host controllers or no dedicated device command queue for any host
> controller.

The motivation of this patch is to adapt UFS driver to different host
hardware. IMHO, some host may implement a dedicated (extra) queue for
dev commands but not all does. In general, one hw queue per CPU (IO
queue) topology is desired, hence sharing a hw queue is inevitable on those host without a dedicated dev cmd queue. The host with dedicated queue will keep the benifit of having fast channel for dev commands.

Thanks
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 31df052fbc41..911dc1cbe4bb 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -16,7 +16,8 @@ 
 #define MAX_QUEUE_SUP GENMASK(7, 0)
 #define UFS_MCQ_MIN_RW_QUEUES 2
 #define UFS_MCQ_MIN_READ_QUEUES 0
-#define UFS_MCQ_NUM_DEV_CMD_QUEUES 1
+#define UFS_MCQ_MAX_DEV_CMD_QUEUES 1
+#define UFS_MCQ_MIN_DEV_CMD_QUEUES 0
 #define UFS_MCQ_MIN_POLL_QUEUES 0
 #define QUEUE_EN_OFFSET 31
 #define QUEUE_ID_OFFSET 16
@@ -75,6 +76,23 @@  module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644);
 MODULE_PARM_DESC(poll_queues,
 		 "Number of poll queues used for r/w. Default value is 1");
 
+static int dev_cmd_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+	return param_set_uint_minmax(val, kp,
+				     UFS_MCQ_MIN_DEV_CMD_QUEUES,
+				     UFS_MCQ_MAX_DEV_CMD_QUEUES);
+}
+
+static const struct kernel_param_ops dev_cmd_queue_count_ops = {
+	.set = dev_cmd_queue_count_set,
+	.get = param_get_uint,
+};
+
+unsigned int dev_cmd_queues = 1;
+module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644);
+MODULE_PARM_DESC(dev_cmd_queues,
+		 "Number of queues used for dev command. Default value is 1");
+
 /**
  * ufshcd_mcq_config_mac - Set the #Max Activ Cmds.
  * @hba: per adapter instance
@@ -109,7 +127,7 @@  struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 	u32 hwq = blk_mq_unique_tag_to_hwq(utag);
 
 	/* uhq[0] is used to serve device commands */
-	return &hba->uhq[hwq + UFSHCD_MCQ_IO_QUEUE_OFFSET];
+	return &hba->uhq[hwq + dev_cmd_queues];
 }
 
 /**
@@ -152,7 +170,7 @@  static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 
 	hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities);
 
-	tot_queues = UFS_MCQ_NUM_DEV_CMD_QUEUES + read_queues + poll_queues +
+	tot_queues = dev_cmd_queues + read_queues + poll_queues +
 			rw_queues;
 
 	if (hba_maxq < tot_queues) {
@@ -161,7 +179,7 @@  static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 		return -EOPNOTSUPP;
 	}
 
-	rem = hba_maxq - UFS_MCQ_NUM_DEV_CMD_QUEUES;
+	rem = hba_maxq - dev_cmd_queues;
 
 	if (rw_queues) {
 		hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues;
@@ -187,7 +205,7 @@  static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 	for (i = 0; i < HCTX_MAX_TYPES; i++)
 		host->nr_hw_queues += hba->nr_queues[i];
 
-	hba->nr_hw_queues = host->nr_hw_queues + UFS_MCQ_NUM_DEV_CMD_QUEUES;
+	hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queues;
 	return 0;
 }
 
@@ -423,8 +441,11 @@  int ufshcd_mcq_init(struct ufs_hba *hba)
 
 	/* The very first HW queue serves device commands */
 	hba->dev_cmd_queue = &hba->uhq[0];
-	/* Give dev_cmd_queue the minimal number of entries */
-	hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES;
+	if (dev_cmd_queues) {
+		/* Give dedicated dev_cmd_queue the minimal number of entries */
+		hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES;
+	}
+
 
 	host->host_tagset = 1;
 	return 0;
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 529f8507a5e4..bad611ac390e 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -78,7 +78,6 @@  struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 				       struct ufs_hw_queue *hwq);
 
-#define UFSHCD_MCQ_IO_QUEUE_OFFSET	1
 #define SD_ASCII_STD true
 #define SD_RAW false
 int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
@@ -287,6 +286,7 @@  static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+extern unsigned int dev_cmd_queues;
 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 acae4e194ec4..f2c62a41bb33 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5513,7 +5513,7 @@  static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	struct ufs_hw_queue *hwq;
 
 	if (is_mcq_enabled(hba)) {
-		hwq = &hba->uhq[queue_num + UFSHCD_MCQ_IO_QUEUE_OFFSET];
+		hwq = &hba->uhq[queue_num + dev_cmd_queues];
 
 		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
 	}